Add Server Instance Feature #1450

SimonHoenscheid · 2023-06-21T15:43:01Z

Summary

This PR adds server instances to the module.

Additional Context

Currently the server_instances define relies on the server class providing a baseline
instances with different versions are not possible, except for upgrade scenarios (e.g 13 was managed before, 14 is the new managed version)

Related Issues (if any)

SimonHoenscheid · 2023-09-25T20:04:41Z

I think this looks good at the moment. Can you maybe add an acceptance test to spin up multiple instances?

acceptance test added

instance directories is now a hash, update example

* postgresql::server::config_entry in postgresql::server::instance::config * easier path management in postgresql::server::config_entry * add taget to $pg_hba_conf_defaults

update provider docs

* add other missing parameters * add missing documentation

update spec tests

Ramesh7

The code looks good but need more review from postgres experts.

smortex

Looks globally good. I have some concerns about the interface which feels very complex and repetitive but I don't think there is much we can do unfortunately.

smortex · 2023-10-06T11:03:52Z

README.md

+# stop default main instance
+postgresql::server::service_ensure: "stopped"
+postgresql::server::service_enable: false


This should go in the above profile rather than in Hiera.

smortex · 2023-10-06T11:16:15Z

README.md

+    config_settings:
+      pg_hba_conf_path: "/opt/pgsql/data/13/test1/pg_hba.conf"
+      postgresql_conf_path: "/opt/pgsql/data/13/test1/postgresql.conf"
+      pg_ident_conf_path: "/opt/pgsql/data/13/test1/pg_ident.conf"
+      datadir: "/opt/pgsql/data/13/test1"
+      service_name: "postgresql@13-test1"
+      port: 5433
+      pg_hba_conf_defaults: false
+    service_settings:
+      service_name: "postgresql@13-test1"
+      service_status: "systemctl status postgresql@13-test1.service"
+      service_ensure: "running"
+      service_enable: true
+    initdb_settings:
+      auth_local: "peer"
+      auth_host: "md5"
+      needs_initdb: true
+      datadir: "/opt/pgsql/data/13/test1"
+      encoding: "UTF-8"
+      lc_messages: "en_US.UTF8"
+      locale: "en_US.UTF8"
+      data_checksums: false
+      group: "postgres"
+      user: "postgres"
+      username: "ins_test1"
+    config_entries:
+      authentication_timeout:
+        value: "1min"
+        comment: "a test"
+      log_statement_stats:
+        value: "off"
+      autovacuum_vacuum_scale_factor:
+        value: 0.3
+    databases:
+      testdb1:
+        encoding: "UTF8"
+        locale: "en_US.UTF8"
+        owner: "dba_test1"
+      testdb2:
+        encoding: "UTF8"
+        locale: "en_US.UTF8"
+        owner: "dba_test1"
+    roles:
+      "ins_test1":
+        superuser: true
+        login: true
+      "dba_test1":
+        createdb: true
+        login: true
+      "app_test1":
+        login: true
+      "rep_test1":
+        replication: true
+        login: true
+      "rou_test1":
+        login: true
+    pg_hba_rules:
+      "local all INSTANCE user":
+        type: "local"
+        database: "all"
+        user: "ins_test1"
+        auth_method: "peer"
+        order: 1
+      "local all DB user":
+        type: "local"
+        database: "all"
+        user: "dba_test1"
+        auth_method: "peer"
+        order: 2
+      "local all APP user":
+        type: "local"
+        database: "all"
+        user: "app_test1"
+        auth_method: "peer"
+        order: 3
+      "local all READONLY user":
+        type: "local"
+        database: "all"
+        user: "rou_test1"
+        auth_method: "peer"
+        order: 4
+      "remote all INSTANCE user PGADMIN server":
+        type: "host"
+        database: "all"
+        user: "ins_test1"
+        address: "192.168.22.131/32"
+        auth_method: "md5"
+        order: 5
+      "local replication INSTANCE user":
+        type: "local"
+        database: "replication"
+        user: "ins_test1"
+        auth_method: "peer"
+        order: 6
+      "local replication REPLICATION user":
+        type: "local"
+        database: "replication"
+        user: "rep_test1"
+        auth_method: "peer"
+        order: 7


These 120 lines of boilerplate are scary 😱

I guess that if it was trivial it would have been done, but just to be sure, we have no way to skip some parameters that feel redundant (e.g. pass a datadir and infer the path to config files, generate the service name based on the instance name, etc)? That would require to change a lot of parameter default values and use the value of previous parameters instead of taking a value directly from params.pp?

tuxmea · 2023-10-06T11:39:28Z

README.md

+      "/opt/pgsql/log/13":
+        ensure: directory
+      "/opt/pgsql/log/13/test1":
+        ensure: directory


Can we place the directory names in an array? This would reduce the number of lines.

Another option would be to make use of the dirtree module and the dirtree type/provider inside the instance defined type.
Using dirtree option should be done in a different PR as this will raise different discussion.

tuxmea · 2023-10-06T11:41:09Z

README.md

+      "/opt/pgsql/log/13/test1":
+        ensure: directory
+    config_settings:
+      pg_hba_conf_path: "/opt/pgsql/data/13/test1/pg_hba.conf"


Here we have to replicate the version in many occasions.
Maybe version should become a hiera data, so it can be reused where needed.
Hardcoding version might make sense, if one is able to install multiple postgresql versions in parallel.

smortex · 2023-11-11T21:48:44Z

lib/puppet/provider/postgresql_conf/ruby.rb

+
+Puppet::Type.type(:postgresql_conf).provide(:ruby) do
+  desc 'Set keys, values and comments in a postgresql config file.'
+  confine kernel: 'Linux'


(re-adding this comment because I was not reporting it where I though)

This seems to break the module on all non-linux OS:

root@agrajag /usr/home/romain # facter os { architecture => "amd64", family => "FreeBSD", hardware => "amd64", name => "FreeBSD", release => { branch => "RC4", full => "14.0-RC4", major => "14", minor => "0" } } root@agrajag /usr/home/romain # puppet agent -t -E romain --tags Postgresql [...] Notice: /Stage[main]/Postgresql::Server::Service/Postgresql::Server::Instance::Service[main]/Anchor[postgresql::server::service::begin::main]: Dependency Postgresql_conf[port_for_instance_main] has failures: true Notice: /Stage[main]/Postgresql::Server::Service/Postgresql::Server::Instance::Service[main]/Anchor[postgresql::server::service::begin::main]: Dependency Postgresql_conf[password_encryption_for_instance_main] has failures: true Notice: /Stage[main]/Postgresql::Server::Service/Postgresql::Server::Instance::Service[main]/Anchor[postgresql::server::service::begin::main]: Dependency Postgresql_conf[data_directory_for_instance_main] has failures: true [...] Error: Could not find a suitable provider for postgresql_conf

This is the outcome of a large refactoring in puppetlabs#1450 and discussions on IRC and slack. We noticed that the behaviour of `$connect_settings` is currently inconsistent. Sometimes it's preferred over explicity parameters, sometimes not. Reminder: The idea of `$connect_settings` is to provide a hash with environment variable to tell puppet to manage a remote database instead of a local instance. One example is: https://github.com/puppetlabs/puppetlabs-postgresql/blob/93386b48861ff41d5f47033afee592e0506526c5/manifests/server/grant.pp#L80-L86 ``` if $port { $port_override = $port } elsif 'PGPORT' in $connect_settings { $port_override = undef } else { $port_override = $postgresql::server::port } ``` Here the local $port parameter is preferred over `$connect_settings`. The problem is that we now cannot set a default in the resource for `$port`. The default is hardcoded to `$postgresql::server::port`. This becomes. Other classes handle it in a different way: https://github.com/puppetlabs/puppetlabs-postgresql/blob/93386b48861ff41d5f47033afee592e0506526c5/manifests/server/database.pp#L41-L46 ``` if 'PGPORT' in $connect_settings { $port_override = undef } else { $port_override = $port } ``` Here `$connct_settings` is checked first. It defaults to an empty hash. If `PGPORT` isn't in it, `$port` is used. The logic is shorter and enables us to expose $port as a parameter with a default value. With this approach users can decide if they pass `$port` or `$connect_settings`. At the moment the remote database support is broken because the logic to parse `$connect_settings` isn't consistent. The goal of this PR is to unify the logic and prefer `$connect_settings` if it's provided by the user.

ia-content added the community label Jun 21, 2023

SimonHoenscheid force-pushed the instances_define branch 4 times, most recently from c8e64a1 to 8b6bb15 Compare June 28, 2023 14:57

SimonHoenscheid force-pushed the instances_define branch 20 times, most recently from 0a95726 to b3cbf1e Compare July 6, 2023 12:32

jordanbreen28 added the feature label Jul 7, 2023

SimonHoenscheid force-pushed the instances_define branch 4 times, most recently from 7191732 to d053e38 Compare August 2, 2023 07:00

SimonHoenscheid force-pushed the instances_define branch 7 times, most recently from ce41078 to 70170bc Compare September 25, 2023 15:50

SimonHoenscheid added 2 commits September 27, 2023 14:51

initial server instance define

144d448

instance directories is now a hash, update example

add default settings for paths

f6ff7c2

* postgresql::server::config_entry in postgresql::server::instance::config * easier path management in postgresql::server::config_entry * add taget to $pg_hba_conf_defaults

SimonHoenscheid force-pushed the instances_define branch from 70170bc to f1e9af6 Compare September 27, 2023 12:59

SimonHoenscheid added 6 commits September 27, 2023 16:22

New postgresql_conf provider, handles multiple configs

179472b

update provider docs

postgresql::server: default username parameter to $user

1dcead6

patch postgres_psql calls,add instance parameter

3152372

* add other missing parameters * add missing documentation

extend systemd define to handle instance systemd files

5b829f4

Add spec test for instance test1

a5185d9

update spec tests

add acceptence test for instance test1

ae5c637

SimonHoenscheid force-pushed the instances_define branch from f1e9af6 to ae5c637 Compare September 27, 2023 14:22

Ramesh7 approved these changes Sep 29, 2023

View reviewed changes

smortex approved these changes Oct 6, 2023

View reviewed changes

tuxmea approved these changes Oct 6, 2023

View reviewed changes

Ramesh7 merged commit ef2e9bc into puppetlabs:main Oct 10, 2023
37 checks passed

jordanbreen28 removed the feature label Oct 10, 2023

smortex reviewed Nov 11, 2023

View reviewed changes

smortex mentioned this pull request Nov 11, 2023

Module broken on non-Linux operating system #1550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Server Instance Feature #1450

Add Server Instance Feature #1450

SimonHoenscheid commented Jun 21, 2023 •

edited

Loading

SimonHoenscheid commented Sep 25, 2023

Ramesh7 left a comment

smortex left a comment

smortex Oct 6, 2023

smortex Oct 6, 2023

tuxmea Oct 6, 2023

tuxmea Oct 6, 2023

smortex Nov 11, 2023

Add Server Instance Feature #1450

Add Server Instance Feature #1450

Conversation

SimonHoenscheid commented Jun 21, 2023 • edited Loading

Summary

Additional Context

Related Issues (if any)

SimonHoenscheid commented Sep 25, 2023

Ramesh7 left a comment

Choose a reason for hiding this comment

smortex left a comment

Choose a reason for hiding this comment

smortex Oct 6, 2023

Choose a reason for hiding this comment

smortex Oct 6, 2023

Choose a reason for hiding this comment

tuxmea Oct 6, 2023

Choose a reason for hiding this comment

tuxmea Oct 6, 2023

Choose a reason for hiding this comment

smortex Nov 11, 2023

Choose a reason for hiding this comment

SimonHoenscheid commented Jun 21, 2023 •

edited

Loading