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

[myii]

NOTE: This [first commit] is not my work.

In #236 (comment), I specifically mentioned that this solution was provided by @RobRuana in #216. The specific commit:

• ef87358

I noticed that this issue was still open, so I have set @RobRuana as the author of the commit in an attempt to resolve this issue as well as attribute it accordingly. If this is inappropriate in any way, I am most happy to close this PR.

[vutny]

Hi @myii !

Thanks for your PR. The logic in the formula around restarting/reloading the service remains quite complex.

I have few suggestions:

• First you need to remove the requisite for postgresql-pg_hba from postgresql-running state. It becomes excrescent. Also, a restarting is mandatory on MacOS anyway, so you might need to check that.
• Better to move the restarting state at the bottom of the SLS file. There are SELinux related states below the current position. And when you restart the service early, later postgresql-running state will not take changes and the tablespaces could not be accessed.
• This is optional, but you may think about restarting PostgreSQL only when it is actually running before attempting to make a cold start. This way you avoid starting and immediate restarting during initial state run or when the cluster was shut down.

The

[myii]

@vutny Thanks for the prompt review and feedback -- excellent again, as usual.

---

Discussion about commit added

I've added another commit to the PR to address the issues you've raised. But don't merge this yet -- I've added it primarily for discussion on how best to move forwards with this. Let me start by replying point by point.

Thanks for your PR. The logic in the formula around restarting/reloading the service remains quite complex.

I believe we can address and resolve this by criticising the commit I've just added. I say that because I implemented each sub-issue in the most direct manner, in order to help highlight the overlapping logic.

• First you need to remove the requisite for postgresql-pg_hba from postgresql-running state. It becomes excrescent.

• Good catch -- done.

• ... Also, a restarting is mandatory on MacOS anyway, so you might need to check that.

• To avoid an unnecessary second restart on MacOS, I've explicitly separated the end point for it compared to the other OSes:
  • MacOS: All restarts occur under service: postgresql-running and module: postgresql-service-restart is never used.
  • !MacOS: Same as before, reload remains in service: postgresql-running and all restarts remain under module: postgresql-service-restart.

• Better to move the restarting state at the bottom of the SLS file. There are SELinux related states below the current position. And when you restart the service early, later postgresql-running state will not take changes and the tablespaces could not be accessed.

• Done.

• This is optional, but you may think about restarting PostgreSQL only when it is actually running before attempting to make a cold start. This way you avoid starting and immediate restarting during initial state run or when the cluster was shut down.

• Added the require requisite.

The

• Was there anything else here you were going to say?

---

Considerations moving forward

Current logic (after this commit)

• service: postgresql-running
  • (a) MacOS: Watch cmd: postgresql-cluster-prepared => restart
  • (b) MacOS: Watch file: postgresql-conf => restart
  • (c) MacOS: Watch file: postgresql-pg_hba => restart
  • (d) MacOS: Watch file: postgresql-pg_ident => restart
  • (e) !MacOS: Watch file: postgresql-pg_ident => reload
• module: postgresql-service-restart
  • (f) !MacOS: Watch cmd: postgresql-cluster-prepared => restart
  • (g) !MacOS: Watch file: postgresql-conf => restart
  • (h) !MacOS: Watch file: postgresql-pg_hba => restart

Proposed logic

Key change:

• module: postgresql-service-restart to become module: postgresql-service-reload (i.e. using service.reload) -- but keeping it in the same place, to run after the service.restart.

Leading to:

• service: postgresql-running
  • (a) + (f) All OSes: Watch cmd: postgresql-cluster-prepared => restart
  • (b) + (g) All OSes: Watch file: postgresql-conf => restart
  • (c) + (h) All OSes: Watch file: postgresql-pg_hba => restart
  • (d) MacOS: Watch file: postgresql-pg_ident => restart
• module: postgresql-service-reload
  • (e) !MacOS: Watch file: postgresql-pg_ident => reload

[myii]

@vutny Leaving the first commit as-is (i.e. commit by @RobRuana), suggesting something like the following:

@@ -84,7 +84,7 @@
       - pkg: postgresql-server
       - file: postgresql-cluster-prepared
     - watch_in:
-      - module: postgresql-service-restart
+      - service: postgresql-running
 {%- endif %}
 
 postgresql-config-dir:
@@ -143,7 +143,7 @@
       - file: postgresql-conf-comment-port
       {%- endif %}
     - watch_in:
-      - module: postgresql-service-restart
+      - service: postgresql-running
 
 {%- endif %}
 
@@ -174,14 +174,7 @@
     - require:
       - file: postgresql-config-dir
     - watch_in:
-      - module: postgresql-service-restart
+      - service: postgresql-running
-
-# 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 }}
 
 {%- set pg_ident_path = salt['file.join'](postgres.conf_dir, 'pg_ident.conf') %}
 
@@ -214,6 +207,12 @@
       {%- else %}
       - file: postgresql-cluster-prepared
       {%- endif %}
+    - watch_in:
+      {%- if grains.os not in ('MacOS',) %}
+      - module: postgresql-service-reload
+      {%- else %}
+      - service: postgresql-running
+      {%- endif %}
 
 {%- for name, tblspace in postgres.tablespaces|dictsort() %}
 
@@ -262,11 +261,14 @@
   service.running:
     - name: {{ postgres.service }}
     - enable: True
-   {% if grains.os not in ('MacOS',) %}
-    - reload: True
-   {% endif %}
-    - watch:
-      - file: postgresql-pg_hba
-      - file: postgresql-pg_ident
+
+{%- if grains.os not in ('MacOS',) %}
+postgresql-service-reload:
+  module.wait:
+    - name: service.reload
+    - m_name: {{ postgres.service }}
+    - require:
+      - service: postgresql-running
+{%- endif %}

Edit: Minimised some of the diff noise.

[RobRuana]

Thanks for picking this up and running with it!

[myii]

@RobRuana You're welcome. Thanks for responding, nice to know that you're OK with it. Don't hesitate to shout if you see anything going awry. git blame is going to show your name as part of this PR...!

[vutny]

@myii That diff you posted is totally awesome, looks much better and consistent. Like it so much.

It would be great to include it as part of this PR.

[myii]

@myii That diff you posted is totally awesome, looks much better and consistent. Like it so much.

It would be great to include it as part of this PR.

@vutny Great -- so my plan is to rebase this as the second commit to lose the one that I used to "explain things". Meaning, two commits in this PR altogether:

1. Keep cce4cbf as the first commit.
2. Use the diff as the second commit.

So we're agreed to change the module service.restart to service.reload, right?

[vutny]

So we're agreed to change the module service.restart to service.reload, right?

This is a right thing to do.

[myii]

@vutny OK, I'll rebase and push within the next few minutes. Thanks again for reviewing.

[myii]

@vutny OK, have pushed to replace the previous second commit. Let me know if all OK. Thanks.

[myii]

@vutny Found a separate problem while testing but a small fix so added another commit here.

Using the identity_map from pillar.example:

postgres-formula/pillar.example

Lines 78 to 80 in c213b30

	identity_map:
	- ['users_as_appuser', 'jdoe', 'connuser']
	- ['users_as_appuser', 'jsmith', 'connuser']

Resulted in an error:

     Comment: Unable to manage file: Jinja error: unsupported format string passed to list.__format__
              Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/utils/templates.py", line 380, in render_jinja_tmpl
                  output = template.render(**decoded_context)
                File "/usr/lib/python3/dist-packages/jinja2/asyncsupport.py", line 76, in render
                  return original_render(self, *args, **kwargs)
                File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 1008, in render
                  return self.environment.handle_exception(exc_info, True)
                File "/usr/lib/python3/dist-packages/jinja2/environment.py", line 780, in handle_exception
                  reraise(exc_type, exc_value, tb)
                File "/usr/lib/python3/dist-packages/jinja2/_compat.py", line 37, in reraise
                  raise value.with_traceback(tb)
                File "<template>", line 50, in top-level template code
              TypeError: unsupported format string passed to list.__format__
              
              ; line 50
              
              ---
              [...]
              # ----------------------------------
              
              # MAPNAME       SYSTEM-USERNAME         PG-USERNAME
              
              {%- for mapping in mappings %}
              {{ '{0:<15} {1:<22} {2}'.format(mapping) -}}    <======================
              {% endfor %}
              
              Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/utils/templates.py", line 380, in render_jinja_tmpl
                  output = template.render(**decoded_context)
              [...]

So fixed the pg_ident.conf.j2 template to unpack the mapping list.

[vutny]

Looks great @myii !

Thank you for addressing this. Now the code is much more readable and you have fixed many use cases around config-to-service-apply logic. Well done.

[myii]

@vutny You're welcome -- wouldn't have even considered making these extra changes if it wasn't for your excellent feedback.

[aboe76]

so @vutny and @myii lgtm?

[vutny]

You got my approval @aboe76 .

[myii]

@aboe76 Ready to go from my end.

One request: if you are planning to squash the commits, could you please leave the first commit separate, since that needs to remain attributed to @RobRuana?

Thanks in advance.

[aboe76]

@myii merged it

[myii]

@aboe76 Thanks, appreciate it.