Fix/debian cluster for the monitor (#681)

* Fix pg_autoctl do pgsetup ... with reading the configuration file.

That fixes a bug where we expect port 5432 (default) to be found in the
Postgres pid file even though the pg_autoctl node has been created with a
specific port number (such as --pgport 6000).

* Discover and fix debian clusters for the monitor too.

In passing, review the test suite for the debian integration and make it
work smoothly when it was almost broken before.

Author: DimCitus

src/bin/pg_autoctl/cli_do_misc.c

@@ -234,15 +234,19 @@ keeper_cli_pgsetup_pg_ctl(int argc, char **argv)
 void
 keeper_cli_pgsetup_discover(int argc, char **argv)
 {
-	bool missingPgdataOk = true;
-	PostgresSetup pgSetup = { 0 };
+	ConfigFilePaths pathnames = { 0 };
+	LocalPostgresServer postgres = { 0 };
+	PostgresSetup *pgSetup = &(postgres.postgresSetup);
 
-	if (!pg_setup_init(&pgSetup, &keeperOptions.pgSetup, true, true))
+	if (!cli_common_pgsetup_init(&pathnames, pgSetup))
 	{
-		exit(EXIT_CODE_PGCTL);
+		/* errors have already been logged */
+		exit(EXIT_CODE_BAD_CONFIG);
 	}
 
-	if (!pg_controldata(&pgSetup, missingPgdataOk))
+	bool missingPgdataOk = true;
+
+	if (!pg_controldata(pgSetup, missingPgdataOk))
 	{
 		exit(EXIT_CODE_PGCTL);
 	}
@@ -252,7 +256,7 @@ keeper_cli_pgsetup_discover(int argc, char **argv)
 		fformat(stdout, "Node Name:          %s\n", keeperOptions.hostname);
 	}
 
-	fprintf_pg_setup(stdout, &pgSetup);
+	fprintf_pg_setup(stdout, pgSetup);
 }
 
 
@@ -263,19 +267,22 @@ keeper_cli_pgsetup_discover(int argc, char **argv)
 void
 keeper_cli_pgsetup_is_ready(int argc, char **argv)
 {
-	PostgresSetup pgSetup = { 0 };
-	bool pgIsNotRunningIsOk = false;
+	ConfigFilePaths pathnames = { 0 };
+	LocalPostgresServer postgres = { 0 };
+	PostgresSetup *pgSetup = &(postgres.postgresSetup);
 
-	if (!pg_setup_init(&pgSetup, &keeperOptions.pgSetup, true, true))
+	if (!cli_common_pgsetup_init(&pathnames, pgSetup))
 	{
-		exit(EXIT_CODE_PGCTL);
+		/* errors have already been logged */
+		exit(EXIT_CODE_BAD_CONFIG);
 	}
 
 	log_debug("Initialized pgSetup, now calling pg_setup_is_ready()");
 
-	bool pgIsReady = pg_setup_is_ready(&pgSetup, pgIsNotRunningIsOk);
+	bool pgIsNotRunningIsOk = false;
+	bool pgIsReady = pg_setup_is_ready(pgSetup, pgIsNotRunningIsOk);
 
-	log_info("Postgres status is: \"%s\"", pmStatusToString(pgSetup.pm_status));
+	log_info("Postgres status is: \"%s\"", pmStatusToString(pgSetup->pm_status));
 
 	if (pgIsReady)
 	{
@@ -293,18 +300,22 @@ void
 keeper_cli_pgsetup_wait_until_ready(int argc, char **argv)
 {
 	int timeout = 30;
-	PostgresSetup pgSetup = { 0 };
 
-	if (!pg_setup_init(&pgSetup, &keeperOptions.pgSetup, true, true))
+	ConfigFilePaths pathnames = { 0 };
+	LocalPostgresServer postgres = { 0 };
+	PostgresSetup *pgSetup = &(postgres.postgresSetup);
+
+	if (!cli_common_pgsetup_init(&pathnames, pgSetup))
 	{
-		exit(EXIT_CODE_PGCTL);
+		/* errors have already been logged */
+		exit(EXIT_CODE_BAD_CONFIG);
 	}
 
 	log_debug("Initialized pgSetup, now calling pg_setup_wait_until_is_ready()");
 
-	bool pgIsReady = pg_setup_wait_until_is_ready(&pgSetup, timeout, LOG_INFO);
+	bool pgIsReady = pg_setup_wait_until_is_ready(pgSetup, timeout, LOG_INFO);
 
-	log_info("Postgres status is: \"%s\"", pmStatusToString(pgSetup.pm_status));
+	log_info("Postgres status is: \"%s\"", pmStatusToString(pgSetup->pm_status));
 
 	if (pgIsReady)
 	{
@@ -320,16 +331,19 @@ keeper_cli_pgsetup_wait_until_ready(int argc, char **argv)
 void
 keeper_cli_pgsetup_startup_logs(int argc, char **argv)
 {
-	PostgresSetup pgSetup = { 0 };
+	ConfigFilePaths pathnames = { 0 };
+	LocalPostgresServer postgres = { 0 };
+	PostgresSetup *pgSetup = &(postgres.postgresSetup);
 
-	if (!pg_setup_init(&pgSetup, &keeperOptions.pgSetup, true, true))
+	if (!cli_common_pgsetup_init(&pathnames, pgSetup))
 	{
-		exit(EXIT_CODE_PGCTL);
+		/* errors have already been logged */
+		exit(EXIT_CODE_BAD_CONFIG);
 	}
 
 	log_debug("Initialized pgSetup, now calling pg_log_startup()");
 
-	if (!pg_log_startup(pgSetup.pgdata, LOG_INFO))
+	if (!pg_log_startup(pgSetup->pgdata, LOG_INFO))
 	{
 		exit(EXIT_CODE_PGCTL);
 	}

src/bin/pg_autoctl/debian.c

@@ -62,9 +62,8 @@ static bool disableAutoStart(PostgresConfigFiles *pgConfigFiles);
  * them from default location and modifies paths inside copied postgresql.conf.
  */
 bool
-keeper_ensure_pg_configuration_files_in_pgdata(KeeperConfig *config)
+keeper_ensure_pg_configuration_files_in_pgdata(PostgresSetup *pgSetup)
 {
-	PostgresSetup *pgSetup = &(config->pgSetup);
 	PostgresConfigFiles pgConfigFiles = { 0 };
 
 	if (!debian_find_postgres_configuration_files(pgSetup, &pgConfigFiles))

src/bin/pg_autoctl/debian.h

@@ -62,7 +62,7 @@ typedef struct debian_pathnames
 } DebianPathnames;
 
 
-bool keeper_ensure_pg_configuration_files_in_pgdata(KeeperConfig *config);
+bool keeper_ensure_pg_configuration_files_in_pgdata(PostgresSetup *pgSetup);
 
 
 #endif /* DEBIAN_H */

src/bin/pg_autoctl/keeper_pg_init.c

@@ -110,7 +110,7 @@ keeper_pg_init_and_register(Keeper *keeper)
 
 	if (postgresInstanceExists)
 	{
-		if (!keeper_ensure_pg_configuration_files_in_pgdata(config))
+		if (!keeper_ensure_pg_configuration_files_in_pgdata(pgSetup))
 		{
 			log_fatal("Failed to setup your Postgres instance "
 					  "the PostgreSQL way, see above for details");

src/bin/pg_autoctl/monitor_pg_init.c

@@ -13,6 +13,7 @@
 #include "postgres_fe.h"
 
 #include "cli_common.h"
+#include "debian.h"
 #include "defaults.h"
 #include "ipaddr.h"
 #include "log.h"
@@ -98,6 +99,14 @@ monitor_pg_init(Monitor *monitor)
 
 			return false;
 		}
+
+		/* if we have a debian cluster, re-own the configuration files */
+		if (!keeper_ensure_pg_configuration_files_in_pgdata(&existingPgSetup))
+		{
+			log_fatal("Failed to setup your Postgres instance "
+					  "the PostgreSQL way, see above for details");
+			return false;
+		}
 	}
 	else
 	{

tests/pgautofailover_utils.py

@@ -148,12 +148,17 @@ def pg_createcluster(self, datadir, port=5432):
         create_command = [
             "sudo",
             shutil.which("pg_createcluster"),
-            "-U",
+            "--user",
             os.getenv("USER"),
-            PGVERSION,
-            datadir,
+            "--group",
+            "postgres",
             "-p",
             str(port),
+            PGVERSION,
+            datadir,
+            "--",
+            "--auth-local",
+            "trust",
         ]
 
         print("%s" % " ".join(create_command))
@@ -1612,14 +1617,14 @@ def __init__(
         else:
             self.hostname = str(self.vnode.address)
 
-    def create(self, run=False):
+    def create(self, level="-v", run=False):
         """
         Initializes and runs the monitor process.
         """
         create_args = [
             "create",
             self.role.command(),
-            "-vv",
+            level,
             "--pgdata",
             self.datadir,
             "--pgport",

tests/test_debian_clusters.py

@@ -1,5 +1,6 @@
 import pgautofailover_utils as pgautofailover
 import os.path
+import subprocess
 
 cluster = None
 monitor = None
@@ -16,10 +17,41 @@ def teardown_module():
 
 def test_000_create_monitor():
     global monitor
-    monitor = cluster.create_monitor("/tmp/debian/monitor")
-    monitor.run()
+
+    print()
+    monitor_path = cluster.pg_createcluster("monitor", port=6000)
+
+    postgres_conf_path = os.path.join(monitor_path, "postgresql.conf")
+    # verify postgresql.conf is not in data directory
+    assert not os.path.exists(postgres_conf_path)
+
+    monitor = cluster.create_monitor(monitor_path, port=6000)
+    monitor.create(level="-vv")
+    monitor.run(port=6000)
     monitor.wait_until_pg_is_running()
 
+    # verify postgresql.conf is in data directory now
+    assert os.path.exists(postgres_conf_path)
+
+    pgversion = os.getenv("PGVERSION")
+
+    p = subprocess.run(
+        [
+            "ls",
+            "-ld",
+            monitor_path,
+            "/var/lib/postgresql/%s" % pgversion,
+            "/etc/postgresql/%s" % pgversion,
+            "/etc/postgresql/%s/monitor" % pgversion,
+            "/etc/postgresql/%s/monitor/postgresql.conf" % pgversion,
+            "/etc/postgresql/%s/monitor/pg_hba.conf" % pgversion,
+            "/etc/postgresql/%s/monitor/pg_ident.conf" % pgversion,
+        ],
+        text=True,
+        capture_output=True,
+    )
+    print("%s" % p.stdout)
+
 
 def test_001_custom_single():
     global node1
@@ -37,13 +69,39 @@ def test_001_custom_single():
     # verify postgresql.conf is in data directory now
     assert os.path.exists(postgres_conf_path)
 
+    pgversion = os.getenv("PGVERSION")
+
+    p = subprocess.run(
+        [
+            "ls",
+            "-ld",
+            node1_path,
+            "/var/lib/postgresql/%s" % pgversion,
+            "/etc/postgresql/%s" % pgversion,
+            "/etc/postgresql/%s/debian_node1" % pgversion,
+            "/etc/postgresql/%s/debian_node1/postgresql.conf" % pgversion,
+            "/etc/postgresql/%s/debian_node1/pg_hba.conf" % pgversion,
+            "/etc/postgresql/%s/debian_node1/pg_ident.conf" % pgversion,
+        ],
+        text=True,
+        capture_output=True,
+    )
+    print("%s" % p.stdout)
+
     monitor.print_state()
 
-    node1.destroy()
 
-    # node is stuck in init state, destroy won't remove it from monitor
-    # force remove from monitor
-    monitor.run_sql_query(
-        "select pgautofailover.remove_node('%s', %s)"
-        % (str(node1.vnode.address), str(node1.port))
+def test_002_chmod_debian_data_directory():
+    # debian installs the following ownership and permissions:
+    #
+    # drwxr-xr-x  5 postgres postgres ... /var/lib/postgresql/11
+    # drwx------ 20 docker   postgres ... /var/lib/postgresql/11/monitor
+    # drwx------ 20 docker   postgres ... /var/lib/postgresql/11/debian_node1
+    #
+    # we need to give the postgres group the w on the top-level directory
+    pgversion = os.getenv("PGVERSION")
+
+    p = subprocess.Popen(
+        ["chmod", "go+w", "/var/lib/postgresql/%s" % pgversion]
     )
+    assert p.wait() == 0