Monitor config set postgresql.pg_ctl bug fix (#818)

- removes double-reload of config file during `config set`
- errors out when attempting to update to an invalid pg_ctl
- allows update to an invalid pg_ctl, in case the binary moved
- add new config test (to be backfilled as more changes come up)
- fix code comment in related code

Co-authored-by: Jacob Champion <pchampion@vmware.com>

Author: Rachel Heaton

Makefile

@@ -26,6 +26,7 @@ TESTS_SINGLE += test_create_run
 TESTS_SINGLE += test_create_standby_with_pgdata
 TESTS_SINGLE += test_ensure
 TESTS_SINGLE += test_skip_pg_hba
+TESTS_SINGLE += test_config_get_set
 
 # Tests for SSL
 TESTS_SSL  = test_enable_ssl

src/bin/pg_autoctl/cli_config.c

@@ -583,7 +583,7 @@ cli_monitor_config_get(int argc, char **argv)
 
 
 /*
- * cli_keeper_config_get retrieves the value of a given configuration value,
+ * cli_config_set sets the value of a given configuration value,
  * supporting either a Keeper or a Monitor configuration file.
  */
 static void
@@ -686,8 +686,6 @@ cli_keeper_config_set(int argc, char **argv)
 static void
 cli_monitor_config_set(int argc, char **argv)
 {
-	KeeperConfig kconfig = keeperOptions;
-
 	if (argc != 2)
 	{
 		log_error("Two arguments are expected, found %d", argc);
@@ -698,15 +696,13 @@ cli_monitor_config_set(int argc, char **argv)
 		/* we print out the value that we parsed, as a double-check */
 		char value[BUFSIZE];
 		MonitorConfig mconfig = { 0 };
-		bool missing_pgdata_is_ok = true;
-		bool pg_is_not_running_is_ok = true;
 
-		if (!monitor_config_init_from_pgsetup(&mconfig,
-											  &kconfig.pgSetup,
-											  missing_pgdata_is_ok,
-											  pg_is_not_running_is_ok))
+		mconfig.pgSetup = keeperOptions.pgSetup;
+
+		if (!monitor_config_set_pathnames_from_pgdata(&mconfig))
 		{
-			exit(EXIT_CODE_PGCTL);
+			/* errors have already been logged */
+			exit(EXIT_CODE_INTERNAL_ERROR);
 		}
 
 		/* first write the new configuration settings to file */

tests/pgautofailover_utils.py

@@ -21,6 +21,11 @@
 
 NodeState = namedtuple("NodeState", "reported assigned")
 
+# Append stderr output to default CalledProcessError message
+class CalledProcessError(subprocess.CalledProcessError):
+    def __str__(self):
+        return super().__str__() + "\n\t" + self.stderr
+
 
 class Role(Enum):
     Monitor = 1
@@ -1952,10 +1957,7 @@ def execute(self, name, *args, timeout=COMMAND_TIMEOUT):
 
             self.last_returncode = proc.returncode
             if proc.returncode > 0:
-                raise Exception(
-                    "%s failed\n%s\n%s\n%s"
-                    % (name, " ".join(self.command), out, err)
-                )
+                raise CalledProcessError(proc.returncode, self.cmd, out, err)
 
             return out, err, proc.returncode
 

tests/test_config_get_set.py

@@ -0,0 +1,86 @@
+import pgautofailover_utils as pgautofailover
+from nose.tools import assert_raises, raises, eq_
+
+import os
+import shutil
+import subprocess
+import time
+
+cluster = None
+monitor = None
+node1 = None
+
+
+def setup_module():
+    global cluster
+    cluster = pgautofailover.Cluster()
+
+
+def teardown_module():
+    cluster.destroy()
+
+
+def test_000_create_monitor():
+    global monitor
+    monitor = cluster.create_monitor("/tmp/config_test/monitor")
+    monitor.run()
+
+
+def test_001_init_primary():
+    global node1
+    node1 = cluster.create_datanode("/tmp/config_test/node1")
+    node1.create()
+
+    # the name of the node should be "%s_%d" % ("node", node1.nodeid)
+    eq_(node1.get_nodename(), "node_%d" % node1.get_nodeid())
+
+    # we can change the name on the monitor with pg_autoctl set node metadata
+    node1.set_metadata(name="node a")
+    eq_(node1.get_nodename(), "node a")
+
+    node1.run()
+    assert node1.wait_until_state(target_state="single")
+
+    # we can also change the name directly in the configuration file
+    node1.config_set("pg_autoctl.name", "a")
+
+    # wait until the reload signal has been processed before checking
+    time.sleep(2)
+    eq_(node1.get_nodename(), "a")
+
+
+def test_002_config_set_monitor():
+    pg_ctl = monitor.config_get("postgresql.pg_ctl")
+
+    # set something non-default to assert no side-effects later
+    sslmode = "prefer"
+    monitor.config_set("ssl.sslmode", sslmode)
+
+    # set monitor config postgresql.pg_ctl to something invalid
+    with assert_raises(subprocess.CalledProcessError):
+        monitor.config_set("postgresql.pg_ctl", "invalid")
+
+    # it should not get changed
+    eq_(monitor.config_get("postgresql.pg_ctl"), pg_ctl)
+
+    # try again with a keeper
+    pg_ctl = node1.config_get("postgresql.pg_ctl")
+
+    # set the keeper to something invalid
+    with assert_raises(subprocess.CalledProcessError):
+        node1.config_set("postgresql.pg_ctl", "invalid")
+
+    # it should not get changed
+    eq_(node1.config_get("postgresql.pg_ctl"), pg_ctl)
+
+    # pg_ctl can be moved and `config set` will still operate.
+    shutil.copy(pg_ctl, "/tmp/pg_ctl")
+    monitor.config_set("postgresql.pg_ctl", "/tmp/pg_ctl")
+    # "move" pg_ctl
+    os.remove("/tmp/pg_ctl")
+    monitor.config_set("postgresql.pg_ctl", pg_ctl)
+
+    eq_(monitor.config_get("postgresql.pg_ctl"), pg_ctl)
+
+    # no side effects
+    eq_(monitor.config_get("ssl.sslmode"), sslmode)