From 6d8ad98f42b308c7dcbac146383658c2e3aaf61a Mon Sep 17 00:00:00 2001 From: Martin Schwenke Date: Thu, 15 May 2025 15:20:25 +1000 Subject: [PATCH 1/2] ctdb-tests: Update statd-callout unit test infrastructure Don't cheat. Keep some state about what is happening, similar to what statd_callout and statd_callout_helper are expected to keep. This means hinting arguments to check_shared_storage_statd_state() and check_statd_callout_smnotify() can be dropped. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15939 Signed-off-by: Martin Schwenke Reviewed-by: Anoop C S (cherry picked from commit 85afee0a83dd2f70b90cff4c1e21b865640261fb) --- .../eventscripts/scripts/statd-callout.sh | 66 ++++++++++++------- .../UNIT/eventscripts/statd-callout.001.sh | 2 +- .../UNIT/eventscripts/statd-callout.002.sh | 2 +- .../UNIT/eventscripts/statd-callout.004.sh | 4 +- .../UNIT/eventscripts/statd-callout.005.sh | 4 +- .../UNIT/eventscripts/statd-callout.006.sh | 4 +- 6 files changed, 52 insertions(+), 30 deletions(-) diff --git a/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh b/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh index 10912c5d3e5..5b03ce3bb30 100644 --- a/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh +++ b/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh @@ -23,13 +23,18 @@ setup() if [ "$statd_callout_location" = "$CTDB_STATD_CALLOUT_SHARED_STORAGE" ]; then statd_callout_location="" fi + + state_dir="${CTDB_TEST_TMP_DIR}/statd-callout-state" + mkdir -p "$state_dir" } -ctdb_catdb_format_pairs() +ctdb_catdb_format() { _count=0 - while read -r _k _v; do + while read -r _sip_cip; do + _k="statd-state@${_sip_cip}" + _v="DATETIME" _kn=$(printf '%s' "$_k" | wc -c) _vn=$(printf '%s' "$_v" | wc -c) cat < ${_cip}, MON_NAME=${FAKE_NFS_HOSTNAME}, STATE=${_state} EOF - done + rm -f "$_f" + done done | { ok simple_test_event "notify" @@ -180,9 +186,25 @@ EOF export CTDB_STATD_CALLOUT_CONFIG_FILE case "$event" in - add-client | del-client) + add-client) cmd="${CTDB_SCRIPTS_HELPER_BINDIR}/statd_callout" unit_test "$cmd" "$event" "$@" + ctdb_get_my_public_addresses | + while read -r _ _sip _; do + touch "${state_dir}/mon@${_sip}@${1}" + done + ;; + del-client) + cmd="${CTDB_SCRIPTS_HELPER_BINDIR}/statd_callout" + unit_test "$cmd" "$event" "$@" + ctdb_get_my_public_addresses | + while read -r _ _sip _; do + rm -f "${state_dir}/mon@${_sip}@${1}" + done + ;; + notify) + cmd="${CTDB_SCRIPTS_TOOLS_HELPER_DIR}/statd_callout_helper" + script_test "$cmd" "$event" "$@" ;; *) cmd="${CTDB_SCRIPTS_TOOLS_HELPER_DIR}/statd_callout_helper" diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.001.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.001.sh index 475750041c5..69de3c93b85 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.001.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.001.sh @@ -16,4 +16,4 @@ simple_test_event "startup" simple_test_event "add-client" "192.168.123.45" simple_test_event "update" -check_shared_storage_statd_state "192.168.123.45" +check_shared_storage_statd_state diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.002.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.002.sh index 7e5d7b23621..7d94d9ca881 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.002.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.002.sh @@ -17,4 +17,4 @@ simple_test_event "add-client" "192.168.123.45" simple_test_event "add-client" "192.168.123.46" simple_test_event "update" -check_shared_storage_statd_state "192.168.123.45" "192.168.123.46" +check_shared_storage_statd_state diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh index 1b1bc05490a..adff2934186 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh @@ -16,8 +16,8 @@ simple_test_event "startup" simple_test_event "add-client" "192.168.123.45" simple_test_event "update" -check_shared_storage_statd_state "192.168.123.45" +check_shared_storage_statd_state -check_statd_callout_smnotify "192.168.123.45" +check_statd_callout_smnotify check_shared_storage_statd_state diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh index 5a9274b1402..2c2fb7ecde6 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh @@ -25,8 +25,8 @@ simple_test_event "update" ctdb_set_pnn 0 -check_statd_callout_smnotify "192.168.123.45" +check_statd_callout_smnotify ctdb_set_pnn 1 -check_shared_storage_statd_state "192.168.123.46" +check_shared_storage_statd_state diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh index 1721a5c50b3..ac64718b7fc 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh @@ -25,10 +25,10 @@ simple_test_event "update" ctdb_set_pnn 0 -check_statd_callout_smnotify "192.168.123.45" +check_statd_callout_smnotify ctdb_set_pnn 1 -check_statd_callout_smnotify "192.168.123.46" +check_statd_callout_smnotify check_shared_storage_statd_state -- 2.47.3 From f6f9982ba247531b55b43f6900bc3a5a46912d55 Mon Sep 17 00:00:00 2001 From: Peter Schwenke Date: Tue, 29 Apr 2025 16:33:45 +1000 Subject: [PATCH 2/2] ctdb-scripts: Only send notifies for newly taken IPs We no longer delete shared state (and send notifies) for IPs previously held by the current node. The NFS lock manager won't have released locks for these IPs, so won't generate SM_MON on reclaim attempts. Therefore, there will be no add-client to put them back. We now record newly taken IP addresses in takeip, and only send notifies for those during ipreallocated. The extra notifies were also confusing statd. Update existing tests to always simulate taking all of a node's IPs. This causes no output changes. Test updates confirm the subtleties of the statd_callout_helper behaviour change. These pretend to only take a single IP, so SM_NOTIFY must not be sent for other IPs. Shared state should remain for these other files. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15939 Signed-off-by: Peter Schwenke Signed-off-by: Martin Schwenke Reviewed-by: Anoop C S (cherry picked from commit e4914e6a4f1cb77eebf86c5ab3f241c2a9e5bd05) --- ctdb/config/events/legacy/60.nfs.script | 3 ++ .../eventscripts/scripts/statd-callout.sh | 14 ++++-- .../UNIT/eventscripts/statd-callout.004.sh | 4 ++ .../UNIT/eventscripts/statd-callout.005.sh | 8 ++++ .../UNIT/eventscripts/statd-callout.006.sh | 8 ++++ .../UNIT/eventscripts/statd-callout.008.sh | 42 ++++++++++++++++++ .../UNIT/eventscripts/statd-callout.108.sh | 6 +++ .../UNIT/eventscripts/statd-callout.208.sh | 6 +++ ctdb/tools/statd_callout_helper | 44 ++++++++++++++++--- 9 files changed, 127 insertions(+), 8 deletions(-) create mode 100755 ctdb/tests/UNIT/eventscripts/statd-callout.008.sh create mode 100755 ctdb/tests/UNIT/eventscripts/statd-callout.108.sh create mode 100755 ctdb/tests/UNIT/eventscripts/statd-callout.208.sh diff --git a/ctdb/config/events/legacy/60.nfs.script b/ctdb/config/events/legacy/60.nfs.script index c59a0c18ea8..0146a39170c 100755 --- a/ctdb/config/events/legacy/60.nfs.script +++ b/ctdb/config/events/legacy/60.nfs.script @@ -325,6 +325,9 @@ shutdown) takeip) nfs_callout "$@" || exit $? + if [ -x "${CTDB_HELPER_BINDIR}/statd_callout_helper" ] ; then + "${CTDB_HELPER_BINDIR}/statd_callout_helper" takeip "$3" + fi ctdb_service_set_reconfigure ;; diff --git a/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh b/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh index 5b03ce3bb30..ff8333c16db 100644 --- a/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh +++ b/ctdb/tests/UNIT/eventscripts/scripts/statd-callout.sh @@ -132,8 +132,9 @@ check_statd_callout_smnotify() nfs_load_config - ctdb_get_my_public_addresses | - while read -r _ _sip _; do + find "$state_dir" -name "takeip@${FAKE_CTDB_PNN}@*" | + while read -r _f; do + _sip="${_f##*@}" _prefix="mon@${_sip}@" find "$state_dir" -name "${_prefix}*" | while read -r _f; do @@ -143,7 +144,8 @@ SM_NOTIFY: ${_sip} -> ${_cip}, MON_NAME=${FAKE_NFS_HOSTNAME}, STATE=${_state} EOF rm -f "$_f" done - done | { + done | + sort | { ok simple_test_event "notify" } || exit $? @@ -202,9 +204,15 @@ EOF rm -f "${state_dir}/mon@${_sip}@${1}" done ;; + takeip) + cmd="${CTDB_SCRIPTS_TOOLS_HELPER_DIR}/statd_callout_helper" + script_test "$cmd" "$event" "$@" + touch "${state_dir}/takeip@${FAKE_CTDB_PNN}@${1}" + ;; notify) cmd="${CTDB_SCRIPTS_TOOLS_HELPER_DIR}/statd_callout_helper" script_test "$cmd" "$event" "$@" + rm -f "${state_dir}/takeip@${FAKE_CTDB_PNN}@"* ;; *) cmd="${CTDB_SCRIPTS_TOOLS_HELPER_DIR}/statd_callout_helper" diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh index adff2934186..2ea80129446 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.004.sh @@ -13,6 +13,10 @@ setup "$mode" ok_null simple_test_event "startup" +ctdb_get_my_public_addresses | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done simple_test_event "add-client" "192.168.123.45" simple_test_event "update" diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh index 2c2fb7ecde6..b3dcfcad360 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.005.sh @@ -13,6 +13,10 @@ setup "$mode" ok_null simple_test_event "startup" +ctdb_get_my_public_addresses | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done simple_test_event "add-client" "192.168.123.45" simple_test_event "update" @@ -20,6 +24,10 @@ ctdb_set_pnn 1 ok_null simple_test_event "startup" +ctdb_get_my_public_addresses | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done simple_test_event "add-client" "192.168.123.46" simple_test_event "update" diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh index ac64718b7fc..35a7f6dcd81 100755 --- a/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.006.sh @@ -13,6 +13,10 @@ setup "$mode" ok_null simple_test_event "startup" +ctdb_get_my_public_addresses | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done simple_test_event "add-client" "192.168.123.45" simple_test_event "update" @@ -20,6 +24,10 @@ ctdb_set_pnn 1 ok_null simple_test_event "startup" +ctdb_get_my_public_addresses | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done simple_test_event "add-client" "192.168.123.46" simple_test_event "update" diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.008.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.008.sh new file mode 100755 index 00000000000..73e4c5b35d1 --- /dev/null +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.008.sh @@ -0,0 +1,42 @@ +#!/bin/sh + +. "${TEST_SCRIPTS_DIR}/unit.sh" + +if [ -z "$CTDB_STATD_CALLOUT_SHARED_STORAGE" ]; then + CTDB_STATD_CALLOUT_SHARED_STORAGE="persistent_db" +fi +mode="$CTDB_STATD_CALLOUT_SHARED_STORAGE" + +define_test "${mode} - add-client on different nodes, take 1 IP, notify on both" + +setup "$mode" + +ok_null +simple_test_event "startup" +ctdb_get_1_public_address | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done +simple_test_event "add-client" "192.168.123.45" +simple_test_event "update" + +ctdb_set_pnn 1 + +ok_null +simple_test_event "startup" +ctdb_get_1_public_address | + while read -r _ sip _; do + simple_test_event "takeip" "$sip" + done +simple_test_event "add-client" "192.168.123.46" +simple_test_event "update" + +ctdb_set_pnn 0 + +check_statd_callout_smnotify + +ctdb_set_pnn 1 + +check_statd_callout_smnotify + +check_shared_storage_statd_state diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.108.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.108.sh new file mode 100755 index 00000000000..224b58429d9 --- /dev/null +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.108.sh @@ -0,0 +1,6 @@ +#!/bin/sh + +CTDB_STATD_CALLOUT_SHARED_STORAGE="shared_dir" + +_dir=$(dirname "$0") +. "${_dir}/statd-callout.008.sh" diff --git a/ctdb/tests/UNIT/eventscripts/statd-callout.208.sh b/ctdb/tests/UNIT/eventscripts/statd-callout.208.sh new file mode 100755 index 00000000000..6fc8bf15afe --- /dev/null +++ b/ctdb/tests/UNIT/eventscripts/statd-callout.208.sh @@ -0,0 +1,6 @@ +#!/bin/sh + +CTDB_STATD_CALLOUT_SHARED_STORAGE="none" + +_dir=$(dirname "$0") +. "${_dir}/statd-callout.008.sh" diff --git a/ctdb/tools/statd_callout_helper b/ctdb/tools/statd_callout_helper index b4606470cf8..26364c44b86 100755 --- a/ctdb/tools/statd_callout_helper +++ b/ctdb/tools/statd_callout_helper @@ -80,6 +80,7 @@ create_add_del_client_dir() # script_state_dir set by ctdb_setup_state_dir() # shellcheck disable=SC2154 statd_callout_state_dir="${script_state_dir}/statd_callout" +statd_new_ips_file="${statd_callout_state_dir}/new_ips.txt" # Set default value, if necessary : "${CTDB_STATD_CALLOUT_SHARED_STORAGE:=persistent_db}" @@ -216,9 +217,10 @@ persistent_db_grep_filter="${statd_callout_state_dir}/.grep_filter" persistent_db_make_grep_filter() { + _ips_file="$1" while read -r _ip; do echo "statd-state@${_ip}@" - done <"$CTDB_MY_PUBLIC_IPS_CACHE" >"$persistent_db_grep_filter" + done <"$_ips_file" >"$persistent_db_grep_filter" } update_persistent_db() @@ -231,7 +233,7 @@ update_persistent_db() exit 0 fi - persistent_db_make_grep_filter + persistent_db_make_grep_filter "$CTDB_MY_PUBLIC_IPS_CACHE" # Use cat instead of direct grep since POSIX grep does not # have -h @@ -249,10 +251,14 @@ update_persistent_db() list_records_persistent_db() { - persistent_db_make_grep_filter + persistent_db_make_grep_filter "$statd_new_ips_file" + # Redirect below to /dev/null is because some versions of grep + # appear to not drain the input if the file passed to -f is + # empty (so it matches nothing). This can cause the first sed + # command in the pipeline to exit with EPIPE. $CTDB catdb "$statd_callout_db" | - sed -n -e 's|^key([0-9]*) = "\([^"]*\)".*|\1|p' | + sed -n -e 's|^key([0-9]*) = "\([^"]*\)".*|\1|p' 2>/dev/null | grep -F -f "$persistent_db_grep_filter" | sed -e 's|statd-state@\([^@]*\)@\(.*\)|\1 \2|' @@ -308,11 +314,27 @@ update_shared_dir() : } +save_ip() +{ + _ip_addr=$1 + _f="$statd_new_ips_file" + _lock="${_f}.lock" + _new="${_f}.new.$$" + { + flock --timeout 10 9 || + die "statd_callout_helper save_ip: timeout" + + cat "$_f" 2>/dev/null >"$_new" + echo "$_ip_addr" >>"$_new" + mv "$_new" "$_f" + } 9>"$_lock" +} + list_records_shared_dir() { while read -r _ip; do ls "${statd_callout_shared_dir}/statd-state@${_ip}@"* - done <"$CTDB_MY_PUBLIC_IPS_CACHE" | + done <"${statd_new_ips_file}" | while read -r _f; do if [ ! -f "$_f" ]; then continue @@ -379,6 +401,11 @@ startup() mkdir -p "$statd_callout_state_dir" + # Create an empty file. Some of the code that processes the + # file can't cope with a missing file, which can happen if a + # node doesn't take any IPs between takeover runs. + : >"${statd_new_ips_file}" + "startup_${statd_callout_mode}" "$_config_file" } @@ -424,6 +451,11 @@ update) update ;; +takeip) + _ip_addr=$2 + save_ip "$_ip_addr" + ;; + notify) # we must restart the lockmanager (on all nodes) so that we get # a clusterwide grace period (so other clients don't take out @@ -447,6 +479,8 @@ notify) statd_state="${statd_callout_state_dir}/.statd_state" list_records >"$statd_state" + # Empty the file but don't remove it - see comment in startup() + : >"${statd_new_ips_file}" if [ ! -s "$statd_state" ]; then rm -f "$statd_state" -- 2.47.3