From 13fbdfff0318de46356e895d74e7e45dbe7b35b2 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Mon, 16 Mar 2020 17:23:17 +0100 Subject: [PATCH] QA work following the docs quickstart (#198) * Don't error out when PGPORT does not exists in the environment. We want to use the environment value as a fallback when the user is not providing us with --pgport, but we don't require the value to be there. * Forward the absolute PGDATA change. We need to change to be reflected so that the postgresql configuration file we prepare contains the absolute path to the OpenSSL self-signed certificate, when we create one. * Review the FOPEN_FLAGS_A to include O_RDWR. Otherwise we get an error to create the startup.log file with the initial logs from starting Postgres. * Fix pg_autoctl show uri on the monitor. * Quickstart review with the new code. In particular we need to be careful about using --auth and --ssl-self-signed options when creating the monitor and every other node. * Adding compat write open mode O_TRUNC. --- docs/quickstart.rst | 11 ++-- src/bin/pg_autoctl/cli_show.c | 76 +++++++++++++++++++++++----- src/bin/pg_autoctl/defaults.h | 4 +- src/bin/pg_autoctl/env_utils.c | 12 +++-- src/bin/pg_autoctl/file_utils.c | 3 +- src/bin/pg_autoctl/monitor_config.c | 5 +- src/bin/pg_autoctl/monitor_pg_init.c | 30 ++++++----- src/bin/pg_autoctl/pgsetup.c | 2 +- 8 files changed, 101 insertions(+), 42 deletions(-) diff --git a/docs/quickstart.rst b/docs/quickstart.rst index d9e38283c..3ea27b711 100644 --- a/docs/quickstart.rst +++ b/docs/quickstart.rst @@ -52,6 +52,7 @@ distinctive port (6000): pg_autoctl create monitor \ --auth trust \ + --ssl-self-signed \ --pgdata ./monitor \ --pgport 6000 @@ -90,10 +91,11 @@ we’ll store the primary node’s data files in a small temporary filesystem. # initialize on that disk pg_autoctl create postgres \ --auth trust \ + --ssl-self-signed \ --pgdata /mnt/node_a/data \ --pgport 6010 \ --pgctl /usr/pgsql-11/bin/pg_ctl \ - --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover + --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover?sslmode=require It creates the database in "/mnt/node_a/data" using the port and node specified. Notice the user and database name in the monitor connection @@ -123,10 +125,11 @@ created the primary: pg_autoctl create postgres \ --auth trust \ + --ssl-self-signed \ --pgdata ./node_b \ --pgport 6011 \ --pgctl /usr/pgsql-11/bin/pg_ctl \ - --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover + --monitor postgres://autoctl_node@127.0.0.1:6000/pg_auto_failover?sslmode=require pg_autoctl run --pgdata ./node_b @@ -215,7 +218,7 @@ To discover the url to use in our case, the following command can be used: .. code-block:: bash - pg_autoctl show uri --formation default --pgdata ./monitor + pg_autoctl show uri --pgdata ./monitor Type | Name | Connection String @@ -229,7 +232,7 @@ writes: .. code-block:: bash psql \ - 'postgres://127.0.0.1:6010,127.0.0.1:6011/?target_session_attrs=read-write' + 'postgres://localhost:6010,localhost:6011/postgres?target_session_attrs=read-write&sslmode=require' When nodes A and B were both running, psql would connect to node A because B would be read-only. However now that A is offline and B is diff --git a/src/bin/pg_autoctl/cli_show.c b/src/bin/pg_autoctl/cli_show.c index 96f248e4e..d4bf51ef5 100644 --- a/src/bin/pg_autoctl/cli_show.c +++ b/src/bin/pg_autoctl/cli_show.c @@ -495,23 +495,73 @@ cli_show_uri(int argc, char **argv) static void cli_show_formation_uri(int argc, char **argv) { - KeeperConfig config = keeperOptions; - Monitor monitor = { 0 }; - bool monitorDisabledIsOk = false; + KeeperConfig kconfig = keeperOptions; - if (!keeper_config_read_file_skip_pgsetup(&config, monitorDisabledIsOk)) + switch (ProbeConfigurationFileRole(kconfig.pathnames.config)) { - /* errors have already been logged */ - exit(EXIT_CODE_BAD_CONFIG); - } + case PG_AUTOCTL_ROLE_MONITOR: + { + Monitor monitor = { 0 }; + MonitorConfig mconfig = { 0 }; - if (!monitor_init(&monitor, config.monitor_pguri)) - { - /* errors have already been logged */ - exit(EXIT_CODE_BAD_CONFIG); - } + bool missingPgdataIsOk = true; + bool pgIsNotRunningIsOk = true; + char connInfo[MAXCONNINFO]; - (void) print_monitor_and_formation_uri(&config, &monitor, stdout); + if (!monitor_config_init_from_pgsetup(&mconfig, + &kconfig.pgSetup, + missingPgdataIsOk, + pgIsNotRunningIsOk)) + { + /* errors have already been logged */ + exit(EXIT_CODE_PGCTL); + } + + if (!monitor_config_get_postgres_uri(&mconfig, connInfo, MAXCONNINFO)) + { + /* errors have already been logged */ + exit(EXIT_CODE_BAD_CONFIG); + } + + if (!monitor_init(&monitor, connInfo)) + { + /* errors have already been logged */ + exit(EXIT_CODE_BAD_CONFIG); + } + + (void) print_monitor_and_formation_uri(&kconfig, &monitor, stdout); + break; + } + + case PG_AUTOCTL_ROLE_KEEPER: + { + Monitor monitor = { 0 }; + bool monitorDisabledIsOk = false; + + if (!keeper_config_read_file_skip_pgsetup(&kconfig, + monitorDisabledIsOk)) + { + /* errors have already been logged */ + exit(EXIT_CODE_BAD_CONFIG); + } + + if (!monitor_init(&monitor, kconfig.monitor_pguri)) + { + /* errors have already been logged */ + exit(EXIT_CODE_BAD_CONFIG); + } + + (void) print_monitor_and_formation_uri(&kconfig, &monitor, stdout); + break; + } + + default: + { + log_fatal("Unrecognized configuration file \"%s\"", + kconfig.pathnames.config); + exit(EXIT_CODE_INTERNAL_ERROR); + } + } } diff --git a/src/bin/pg_autoctl/defaults.h b/src/bin/pg_autoctl/defaults.h index 46a76dadf..3b13420e7 100644 --- a/src/bin/pg_autoctl/defaults.h +++ b/src/bin/pg_autoctl/defaults.h @@ -115,10 +115,10 @@ /* * This opens file write only and creates if it doesn't exist. */ -#define FOPEN_FLAGS_W O_WRONLY | O_CREAT +#define FOPEN_FLAGS_W O_WRONLY | O_TRUNC | O_CREAT /* * This opens the file in append mode and creates it if it doesn't exist. */ -#define FOPEN_FLAGS_A O_APPEND | O_CREAT +#define FOPEN_FLAGS_A O_APPEND | O_RDWR | O_CREAT #endif /* DEFAULTS_H */ diff --git a/src/bin/pg_autoctl/env_utils.c b/src/bin/pg_autoctl/env_utils.c index 7097db530..cf08fcff8 100644 --- a/src/bin/pg_autoctl/env_utils.c +++ b/src/bin/pg_autoctl/env_utils.c @@ -26,7 +26,8 @@ env_found_empty(const char *name) char *envvalue = NULL; if (name == NULL || strlen(name) == 0) { - log_error("Failed to get environment setting. NULL or empty variable name is provided"); + log_error("Failed to get environment setting. " + "NULL or empty variable name is provided"); return false; } @@ -50,7 +51,8 @@ env_exists(const char *name) { if (name == NULL || strlen(name) == 0) { - log_error("Failed to get environment setting. NULL or empty variable name is provided"); + log_error("Failed to get environment setting. " + "NULL or empty variable name is provided"); return false; } @@ -78,13 +80,15 @@ get_env_copy_with_fallback(const char *name, char *result, int maxLength, if (name == NULL || strlen(name) == 0) { - log_error("Failed to get environment setting. NULL or empty variable name is provided"); + log_error("Failed to get environment setting. " + "NULL or empty variable name is provided"); return false; } if (result == NULL) { - log_error("Failed to get environment setting. Tried to store in NULL pointer"); + log_error("Failed to get environment setting. " + "Tried to store in NULL pointer"); return false; } diff --git a/src/bin/pg_autoctl/file_utils.c b/src/bin/pg_autoctl/file_utils.c index 9b45bb60b..ec0e1792e 100644 --- a/src/bin/pg_autoctl/file_utils.c +++ b/src/bin/pg_autoctl/file_utils.c @@ -173,8 +173,8 @@ fopen_read_only(const char *filePath) bool write_file(char *data, long fileSize, const char *filePath) { - FILE *fileStream = fopen_with_umask(filePath, "wb", FOPEN_FLAGS_W, 0644); + if (fileStream == NULL) { /* errors have already been logged */ @@ -207,6 +207,7 @@ bool append_to_file(char *data, long fileSize, const char *filePath) { FILE *fileStream = fopen_with_umask(filePath, "ab", FOPEN_FLAGS_A, 0644); + if (fileStream == NULL) { /* errors have already been logged */ diff --git a/src/bin/pg_autoctl/monitor_config.c b/src/bin/pg_autoctl/monitor_config.c index 67a5538f8..fe88a2045 100644 --- a/src/bin/pg_autoctl/monitor_config.c +++ b/src/bin/pg_autoctl/monitor_config.c @@ -578,11 +578,10 @@ monitor_config_set_setting(MonitorConfig *config, bool monitor_config_update_with_absolute_pgdata(MonitorConfig *config) { - PostgresSetup pgSetup = config->pgSetup; + PostgresSetup *pgSetup = &(config->pgSetup); - if (pg_setup_set_absolute_pgdata(&pgSetup)) + if (pg_setup_set_absolute_pgdata(pgSetup)) { - strlcpy(config->pgSetup.pgdata, pgSetup.pgdata, MAXPGPATH); if (!monitor_config_write_file(config)) { /* errors have already been logged */ diff --git a/src/bin/pg_autoctl/monitor_pg_init.c b/src/bin/pg_autoctl/monitor_pg_init.c index a141ba3ae..146edaffd 100644 --- a/src/bin/pg_autoctl/monitor_pg_init.c +++ b/src/bin/pg_autoctl/monitor_pg_init.c @@ -68,15 +68,15 @@ bool monitor_pg_init(Monitor *monitor, MonitorConfig *config) { char configFilePath[MAXPGPATH]; - PostgresSetup pgSetup = config->pgSetup; + PostgresSetup *pgSetup = &(config->pgSetup); - if (directory_exists(pgSetup.pgdata)) + if (directory_exists(pgSetup->pgdata)) { PostgresSetup existingPgSetup = { 0 }; bool missing_pgdata_is_ok = true; bool pg_is_not_running_is_ok = true; - if (!pg_setup_init(&existingPgSetup, &pgSetup, + if (!pg_setup_init(&existingPgSetup, pgSetup, missing_pgdata_is_ok, pg_is_not_running_is_ok)) { @@ -89,7 +89,7 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config) { log_info("Installing pg_auto_failover monitor in existing " "PostgreSQL instance at \"%s\" running on port %d", - pgSetup.pgdata, existingPgSetup.pidFile.port); + pgSetup->pgdata, existingPgSetup.pidFile.port); if (!monitor_install(config->nodename, existingPgSetup, true)) { @@ -105,15 +105,15 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config) if (pg_setup_pgdata_exists(&existingPgSetup)) { log_fatal("PGDATA directory \"%s\" already exists, skipping", - pgSetup.pgdata); + pgSetup->pgdata); return false; } } - if (!pg_ctl_initdb(pgSetup.pg_ctl, pgSetup.pgdata)) + if (!pg_ctl_initdb(pgSetup->pg_ctl, pgSetup->pgdata)) { log_fatal("Failed to initialise a PostgreSQL instance at \"%s\", " - "see above for details", pgSetup.pgdata); + "see above for details", pgSetup->pgdata); return false; } @@ -131,16 +131,16 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config) * We just did the initdb ourselves, so we know where the configuration * file is to be found Also, we didn't start PostgreSQL yet. */ - join_path_components(configFilePath, pgSetup.pgdata, "postgresql.conf"); + join_path_components(configFilePath, pgSetup->pgdata, "postgresql.conf"); /* * When --ssl-self-signed has been used, now is the time to build a * self-signed certificate for the server. We place the certificate and * private key in $PGDATA/server.key and $PGDATA/server.crt */ - if (pgSetup.ssl.createSelfSignedCert) + if (pgSetup->ssl.createSelfSignedCert) { - if (!pg_create_self_signed_cert(&pgSetup, config->nodename)) + if (!pg_create_self_signed_cert(pgSetup, config->nodename)) { log_error("Failed to create SSL self-signed certificate, " "see above for details"); @@ -148,7 +148,7 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config) } } - if (!pg_add_auto_failover_default_settings(&pgSetup, configFilePath, + if (!pg_add_auto_failover_default_settings(pgSetup, configFilePath, monitor_default_settings)) { log_error("Failed to add default settings to \"%s\": couldn't " @@ -157,14 +157,16 @@ monitor_pg_init(Monitor *monitor, MonitorConfig *config) return false; } - if (!pg_ctl_start(pgSetup.pg_ctl, - pgSetup.pgdata, pgSetup.pgport, pgSetup.listen_addresses)) + if (!pg_ctl_start(pgSetup->pg_ctl, + pgSetup->pgdata, + pgSetup->pgport, + pgSetup->listen_addresses)) { log_error("Failed to start postgres, see above"); return false; } - if (!monitor_install(config->nodename, pgSetup, false)) + if (!monitor_install(config->nodename, *pgSetup, false)) { return false; } diff --git a/src/bin/pg_autoctl/pgsetup.c b/src/bin/pg_autoctl/pgsetup.c index 8ae7fa91c..4880033d2 100644 --- a/src/bin/pg_autoctl/pgsetup.c +++ b/src/bin/pg_autoctl/pgsetup.c @@ -1151,7 +1151,7 @@ pgsetup_get_pgport() char pgport_env[NAMEDATALEN]; int pgport = 0; - if (get_env_copy("PGPORT", pgport_env, NAMEDATALEN)) + if (env_exists("PGPORT") && get_env_copy("PGPORT", pgport_env, NAMEDATALEN)) { if (stringToInt(pgport_env, &pgport) && pgport > 0) {