From: Martin Schwenke Date: Fri, 10 May 2024 01:42:26 +0000 (+1000) Subject: ctdb-failover: Split statd_callout add-client/del-client X-Git-Tag: tdb-1.4.11~297 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=415f9f07456e3fd24063e7508d8b2553df020c21;p=thirdparty%2Fsamba.git ctdb-failover: Split statd_callout add-client/del-client rpc.statd is single-threaded and runs its HA callout synchronously. If it is too slow then latency accumulates and rpc.statd's backlog grows. Running a pair of add-client/del-client events with the current code averages ~0.030s in my test environment. This mean that 1000 clients reclaiming locks after failover can easily cause 10s of latency. This could cause rpc.statd to become unresponsive, resulting in a time out for an rpcinfo-based health check of the status service. Split the add-client/del-client events out to a standalone statd_callout executable, written in C, to be used as the HA callout for rpc.statd. All other functions move to statd_callout_helper. Now, running a pair of add-client/del-client events in my test environment averages only ~0.002s. This seems less likely to cause latency problems. The standalone statd_callout executable needs to read a configuration file, which is generated by statd_callout_helper from the "startup" event. It also needs access to a list of currently assigned public IPs. For backward compatibility, during installation a symlink is created from $CTDB_BASE/statd-callout to the new statd_callout, which is installed in the helper directory. Testing this as part of the eventscript unit tests starts to become even more of a hack than it used to be. However, the dependency on stubs and the corresponding setup of fake state makes it hard to move this elsewhere. Signed-off-by: Martin Schwenke Reviewed-by: Amitay Isaacs Autobuild-User(master): Martin Schwenke Autobuild-Date(master): Tue Jun 25 04:24:57 UTC 2024 on atb-devel-224 --- diff --git a/ctdb/config/README b/ctdb/config/README index d28f4f0fc60..3fd1583ecee 100644 --- a/ctdb/config/README +++ b/ctdb/config/README @@ -14,11 +14,6 @@ Selected highlights: Support functions, sourced by eventscripts and other scripts. - statd-callout - - rpc.statd high-availability callout to support lock migration on - failover. - Notes: * All of these scripts are written in POSIX Bourne shell. Please diff --git a/ctdb/config/events/legacy/60.nfs.script b/ctdb/config/events/legacy/60.nfs.script index 867cc72d50c..246a856bca8 100755 --- a/ctdb/config/events/legacy/60.nfs.script +++ b/ctdb/config/events/legacy/60.nfs.script @@ -21,8 +21,8 @@ service_reconfigure() # Restart lock manager, notify clients # shellcheck disable=SC2317 # Called indirectly via check_thresholds() - if [ -x "${CTDB_BASE}/statd-callout" ]; then - "${CTDB_BASE}/statd-callout" notify & + if [ -x "${CTDB_HELPER_BINDIR}/statd_callout_helper" ] ; then + "${CTDB_HELPER_BINDIR}/statd_callout_helper" notify & fi >/dev/null 2>&1 } @@ -281,12 +281,12 @@ nfs_check_rpcinfo() } ################################################################## -# use statd-callout to update NFS lock info +# use statd_callout_helper to update NFS lock info ################################################################## nfs_update_lock_info() { - if [ -x "$CTDB_BASE/statd-callout" ]; then - "$CTDB_BASE/statd-callout" update + if [ -x "$CTDB_HELPER_BINDIR/statd_callout_helper" ] ; then + "$CTDB_HELPER_BINDIR/statd_callout_helper" update fi } @@ -298,8 +298,8 @@ nfs_callout_init "$script_state_dir" case "$1" in startup) - if [ -x "${CTDB_BASE}/statd-callout" ] ; then - "${CTDB_BASE}/statd-callout" startup + if [ -x "${CTDB_HELPER_BINDIR}/statd_callout_helper" ] ; then + "${CTDB_HELPER_BINDIR}/statd_callout_helper" startup fi nfs_callout "$@" || exit $? diff --git a/ctdb/config/functions b/ctdb/config/functions index fbb1e284020..ef79dbf2162 100755 --- a/ctdb/config/functions +++ b/ctdb/config/functions @@ -270,7 +270,7 @@ ctdb_get_ip_address() } # Cache of public IP addresses assigned to this node. This function -# exists mainly so statd-callout does not need to talk to ctdbd, so +# exists mainly so statd_callout does not need to talk to ctdbd, so # can be run as non-root, but it may be used in other places. This # must be updated/refreshed on failover. This is done in # 10.interface, but doing it in "ipreallocated" isn't enough because diff --git a/ctdb/failover/statd_callout.c b/ctdb/failover/statd_callout.c new file mode 100644 index 00000000000..9a85cad7fe2 --- /dev/null +++ b/ctdb/failover/statd_callout.c @@ -0,0 +1,327 @@ +/* + CTDB NFSv3 rpc.statd HA callout + + Copyright 2023, DataDirect Networks, Inc. All rights reserved. + Author: Martin Schwenke + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, see . +*/ + +#include +#include +#include +#include +#include +#include +#include + +/* + * A configuration file, created by statd_callout_helper, containing + * at least 1 line of text. + * + * The first line is the mode. Currently supported modes are: + * + * persistent_db + * + * In this mode, the file contains 2 subsequent lines of text: + * + * path: directory where files should be created + * ips_file: file containing node's currently assigned public IP addresses + */ +#define CONFIG_FILE CTDB_VARDIR "/scripts/statd_callout.conf" + +static const char *progname; + +struct { + enum { + CTDB_SC_MODE_PERSISTENT_DB, + } mode; + union { + struct { + char *path; + char *ips_file; + }; + }; +} config; + +static bool getline_strip(char **restrict lineptr, + size_t *n, + FILE *restrict stream) +{ + bool was_null; + int ret; + + was_null = *lineptr == NULL; + + ret = getline(lineptr, n, stream); + if (ret == -1 || ret <= 2) { + if (was_null) { + free(*lineptr); + *lineptr = NULL; + *n = 0; + } + return false; + } + + if ((*lineptr)[ret - 1] == '\n') { + (*lineptr)[ret - 1] = '\0'; + } + + return true; +} + +static void free_config(void) +{ + switch (config.mode) { + case CTDB_SC_MODE_PERSISTENT_DB: + free(config.path); + config.path = NULL; + free(config.ips_file); + config.ips_file = NULL; + } +} + +static void read_config(void) +{ + const char *config_file = NULL; + FILE *f = NULL; + char *mode = NULL; + size_t n = 0; + bool status; + + /* For testing only */ + config_file = getenv("CTDB_STATD_CALLOUT_CONFIG_FILE"); + if (config_file == NULL || strlen(config_file) == 0) { + config_file = CONFIG_FILE; + } + + f = fopen(config_file, "r"); + if (f == NULL) { + fprintf(stderr, + "%s: unable to open config file (%s)\n", + progname, + config_file); + exit(1); + } + + status = getline_strip(&mode, &n, f); + if (!status) { + fprintf(stderr, + "%s: error parsing mode in %s\n", + progname, + config_file); + exit(1); + } + if (strcmp(mode, "persistent_db") == 0) { + config.mode = CTDB_SC_MODE_PERSISTENT_DB; + } else { + fprintf(stderr, + "%s: unknown mode=%s in %s\n", + progname, + mode, + config_file); + free(mode); + exit(1); + } + free(mode); + + switch (config.mode) { + case CTDB_SC_MODE_PERSISTENT_DB: + status = getline_strip(&config.path, &n, f); + if (!status) { + goto parse_error; + } + + status = getline_strip(&config.ips_file, &n, f); + if (!status) { + goto parse_error; + } + + break; + } + + fclose(f); + return; + +parse_error: + fprintf(stderr, + "%s: error parsing contents of %s\n", + progname, + config_file); + free_config(); + exit(1); +} + +static void for_each_sip(void (*line_func)(const char *sip, const char *cip), + const char *cip) +{ + FILE *f = NULL; + char *line = NULL; + size_t n = 0; + + f = fopen(config.ips_file, "r"); + if (f == NULL) { + fprintf(stderr, + "%s: unable to open IPs file (%s)\n", + progname, + config.ips_file); + exit(1); + } + + for (;;) { + bool status; + + status = getline_strip(&line, &n, f); + if (!status) { + if (!feof(f)) { + fprintf(stderr, + "%s: error parsing contents of %s\n", + progname, + config.ips_file); + free(line); + exit(1); + } + break; + } + + line_func(line, cip); + } + + free(line); + fclose(f); + return; +} + +static void make_path(char *buf, + size_t num, + const char *sip, + const char *cip) +{ + int ret = snprintf(buf, + num, + "%s/statd-state@%s@%s", + config.path, + sip, + cip); + if (ret < 0) { + /* Not possible for snprintf(3)? */ + fprintf(stderr, + "%s: error constructing path %s/statd-state@%s@%s\n", + progname, + config.path, + sip, + cip); + exit(1); + } + if ((size_t)ret >= num) { + fprintf(stderr, + "%s: path too long %s/statd-state@%s@%s\n", + progname, + config.path, + sip, + cip); + exit(1); + } +} + +static void add_client_persistent_db_line(const char *sip, const char *cip) +{ + char path[PATH_MAX]; + FILE *f; + long long datetime; + + make_path(path, sizeof(path), sip, cip); + + datetime = (long long)time(NULL); + + f = fopen(path, "w"); + if (f == NULL) { + fprintf(stderr, + "%s: unable to open for writing %s\n", + progname, + path); + exit(1); + } + fprintf(f, "\"statd-state@%s@%s\" \"%lld\"\n", sip, cip, datetime); + fclose(f); +} + +static void add_client_persistent_db(const char *cip) +{ + for_each_sip(add_client_persistent_db_line, cip); +} + +static void del_client_persistent_db_line(const char *sip, const char *cip) +{ + char path[PATH_MAX]; + FILE *f; + + make_path(path, sizeof(path), sip, cip); + + f = fopen(path, "w"); + if (f == NULL) { + fprintf(stderr, + "%s: unable to open for writing %s\n", + progname, + path); + exit(1); + } + fprintf(f, "\"statd-state@%s@%s\" \"\"\n", sip, cip); + fclose(f); +} + +static void del_client_persistent_db(const char *cip) +{ + for_each_sip(del_client_persistent_db_line, cip); +} + +static void usage(void) +{ + printf("usage: %s: { add-client | del-client } \n", progname); + exit(1); +} + +int main(int argc, const char *argv[]) +{ + const char *event = NULL; + const char *mon_name = NULL; + + progname = argv[0]; + if (argc < 3) { + usage(); + } + + read_config(); + + event = argv[1]; + if (strcmp(event, "add-client") == 0) { + mon_name = argv[2]; + switch (config.mode) { + case CTDB_SC_MODE_PERSISTENT_DB: + add_client_persistent_db(mon_name); + break; + } + } else if (strcmp(event, "del-client") == 0) { + mon_name = argv[2]; + switch (config.mode) { + case CTDB_SC_MODE_PERSISTENT_DB: + del_client_persistent_db(mon_name); + break; + } + } else { + usage(); + } + + free_config(); +} diff --git a/ctdb/tests/UNIT/eventscripts/scripts/60.nfs.sh b/ctdb/tests/UNIT/eventscripts/scripts/60.nfs.sh index 7c3435c8ce0..1a8dab73ded 100644 --- a/ctdb/tests/UNIT/eventscripts/scripts/60.nfs.sh +++ b/ctdb/tests/UNIT/eventscripts/scripts/60.nfs.sh @@ -65,13 +65,6 @@ EOF ;; esac fi - - # This is really nasty. However, when we test NFS we don't - # actually test statd-callout. If we leave it there then left - # over, backgrounded instances of statd-callout will do - # horrible things with the "ctdb ip" stub and cause the actual - # statd-callout tests that follow to fail. - rm "${CTDB_BASE}/statd-callout" } rpc_services_down() diff --git a/ctdb/tests/UNIT/eventscripts/scripts/local.sh b/ctdb/tests/UNIT/eventscripts/scripts/local.sh index 7d667a3c996..d47626472d4 100644 --- a/ctdb/tests/UNIT/eventscripts/scripts/local.sh +++ b/ctdb/tests/UNIT/eventscripts/scripts/local.sh @@ -1,23 +1,5 @@ # Hey Emacs, this is a -*- shell-script -*- !!! :-) -# -# Augment PATH with relevant stubs/ directories. -# - -stubs_dir="${CTDB_TEST_SUITE_DIR}/stubs" -[ -d "${stubs_dir}" ] || die "Failed to locate stubs/ subdirectory" - -# Make the path absolute for tests that change directory -case "$stubs_dir" in -/*) : ;; -*) stubs_dir="${PWD}/${stubs_dir}" ;; -esac - -# Use stubs as helpers -export CTDB_HELPER_BINDIR="$stubs_dir" - -PATH="${stubs_dir}:${PATH}" - export CTDB="ctdb" # Force this to be absolute - event scripts can change directory @@ -36,8 +18,7 @@ setup_ctdb_base "$CTDB_TEST_TMP_DIR" "etc-ctdb" \ debug_locks.sh \ functions \ nfs-checks.d \ - nfs-linux-kernel-callout \ - statd-callout + nfs-linux-kernel-callout export FAKE_CTDB_STATE="${CTDB_TEST_TMP_DIR}/fake-ctdb" mkdir -p "$FAKE_CTDB_STATE" @@ -500,8 +481,9 @@ define_test() esac _s="${script_dir}/${script}" - [ -r "$_s" ] || + if [ ! -r "$_s" ] && [ "$script" != "statd-callout" ]; then die "Internal error - unable to find script \"${_s}\"" + fi case "$script" in *.script) script_short="${script%.script}" ;; @@ -512,6 +494,25 @@ define_test() printf "%-17s %-10s %-4s - %s\n\n" \ "$script_short" "$event" "$_num" "$desc" + # + # Augment PATH with relevant stubs/ directories. + # + + stubs_dir="${CTDB_TEST_SUITE_DIR}/stubs" + [ -d "${stubs_dir}" ] || die "Failed to locate stubs/ subdirectory" + + # Make the path absolute for tests that change directory + case "$stubs_dir" in + /*) : ;; + *) stubs_dir="${PWD}/${stubs_dir}" ;; + esac + + # Use stubs as helpers but remember the original, so + # certain things (e.g. statd_callout) can be found + # + export CTDB_HELPER_BINDIR="$stubs_dir" + PATH="${stubs_dir}:${PATH}" + _f="${CTDB_TEST_SUITE_DIR}/scripts/${script_short}.sh" if [ -r "$_f" ]; then . "$_f" diff --git a/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh b/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh index 1c65c2f98ac..ec75b22b57c 100644 --- a/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh +++ b/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh @@ -2,7 +2,7 @@ setup() { setup_public_addresses ctdb_set_pnn - setup_date "123456789" + setup_date "1234567890" } ctdb_catdb_format_pairs() @@ -25,13 +25,18 @@ EOF echo "Dumped ${_count} records" } +result_filter() +{ + sed -e 's|^\(data(10) = \)".........."$|data(8) = "DATETIME"|' +} + check_ctdb_tdb_statd_state() { ctdb_get_my_public_addresses | while read -r _ _sip _; do for _cip; do cat <"$_config_file" <"$file" - done <"$CTDB_MY_PUBLIC_IPS_CACHE" - ;; - -del-client) - # statd does not tell us from which IP the client disconnected - # so we must add it to all the IPs that we serve - cip="$2" - while read -r sip; do - key="statd-state@${sip}@${cip}" - file="${statd_callout_queue_dir}/${key}" - echo "\"${key}\" \"\"" >"$file" - done <"$CTDB_MY_PUBLIC_IPS_CACHE" - ;; - update) cd "$statd_callout_queue_dir" || die "Failed to change directory to \"${statd_callout_queue_dir}\"" diff --git a/ctdb/wscript b/ctdb/wscript index d4e436931dd..3fbdf7d0425 100644 --- a/ctdb/wscript +++ b/ctdb/wscript @@ -668,6 +668,10 @@ def build(bld): samba-util sys_rw replace tdb''', install_path='${CTDB_HELPER_BINDIR}') + bld.SAMBA_BINARY("statd_callout", + source="failover/statd_callout.c", + install_path="${CTDB_HELPER_BINDIR}") + bld.SAMBA_BINARY('ctdb_takeover_helper', source='server/ctdb_takeover_helper.c', deps='''ctdb-client ctdb-protocol ctdb-util @@ -814,6 +818,16 @@ def build(bld): bld.INSTALL_FILES('${CTDB_HELPER_BINDIR}', 'ctdb_lvs', destname='ctdb_lvs', chmod=MODE_755) + bld.SAMBA_GENERATOR("ctdb-statd-callout-helper", + source="tools/statd_callout_helper", + target="statd_callout_helper", + rule=f"sed {sed_cmdline} ${{SRC}} > ${{TGT}}") + bld.INSTALL_FILES("${CTDB_HELPER_BINDIR}", "statd_callout_helper", + destname="statd_callout_helper", chmod=MODE_755) + + bld.symlink_as(os.path.join(bld.env.CTDB_ETCDIR, "statd-callout"), + os.path.join(bld.env.CTDB_HELPER_BINDIR, "statd_callout")) + def SUBDIR_MODE_callback(arg, dirname, fnames): for f in fnames: fl = os.path.join(dirname, f) @@ -887,7 +901,6 @@ def build(bld): 'debug_locks.sh', 'nfs-linux-kernel-callout', 'notify.sh', - 'statd-callout' ] for t in etc_scripts: