Attachment 18762 Details for Bug 15935 – Patch for v4-21-test

[patch] Patch for v4-21-test

BZ15935-v4-21-test.patch (text/plain), 12.63 KB, created by Martin Schwenke on 2025-10-17 08:28:12 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Schwenke

Created: 2025-10-17 08:28:12 UTC

Size: 12.63 KB

patch

obsolete

>From 7f920206854a294d65fc1fefe8810a1b5ea8b901 Mon Sep 17 00:00:00 2001
>From: Martin Schwenke <mschwenke@ddn.com>
>Date: Fri, 13 Sep 2024 16:21:24 +1000
>Subject: [PATCH 1/5] ctdb-scripts: Reformat with "shfmt -w -p -i 0 -fn"
>
>Massage a couple of lines manually so they're formatted sanely given
>the new indentation.   Re-run shfmt to ensure no further changes.
>
>Best reviewed with "git show -w".
>
>Signed-off-by: Martin Schwenke <mschwenke@ddn.com>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>Reviewed-by: Jerry Heyman <jheyman@ddn.com>
>(cherry picked from commit 3410eddd932b430acc687c81a5dc6e62a0a420a6)
>---
> ctdb/config/events/legacy/10.interface.script | 80 +++++++++----------
> 1 file changed, 39 insertions(+), 41 deletions(-)
>
>diff --git a/ctdb/config/events/legacy/10.interface.script b/ctdb/config/events/legacy/10.interface.script
>index dfd796563fd..137d558b3b7 100755
>--- a/ctdb/config/events/legacy/10.interface.script
>+++ b/ctdb/config/events/legacy/10.interface.script
>@@ -5,7 +5,7 @@
> # this adds/removes IPs from your 
> # public interface
> 
>-[ -n "$CTDB_BASE" ] || \
>+[ -n "$CTDB_BASE" ] ||
> 	CTDB_BASE=$(d=$(dirname "$0") && cd -P "$d" && dirname "$PWD")
> 
> . "${CTDB_BASE}/functions"
>@@ -13,7 +13,7 @@
> load_script_options
> 
> if ! have_public_addresses; then
>-	if [ "$1" = "init" ] ; then
>+	if [ "$1" = "init" ]; then
> 		echo "No public addresses file found"
> 	fi
> 	exit 0
>@@ -32,8 +32,8 @@ monitor_interfaces()
> 	#
> 	# public_ifaces set by get_public_ifaces() above
> 	# shellcheck disable=SC2154
>-	for _iface in $public_ifaces ; do
>-		if interface_monitor "$_iface" ; then
>+	for _iface in $public_ifaces; do
>+		if interface_monitor "$_iface"; then
> 			up_interfaces_found=true
> 			$CTDB setifacelink "$_iface" up >/dev/null 2>&1
> 		else
>@@ -42,11 +42,11 @@ monitor_interfaces()
> 		fi
> 	done
> 
>-	if ! $down_interfaces_found ; then
>+	if ! $down_interfaces_found; then
> 		return 0
> 	fi
> 
>-	if ! $up_interfaces_found ; then
>+	if ! $up_interfaces_found; then
> 		return 1
> 	fi
> 
>@@ -58,63 +58,61 @@ monitor_interfaces()
> }
> 
> # Sets: iface, ip, maskbits
>-get_iface_ip_maskbits ()
>+get_iface_ip_maskbits()
> {
>-    _iface_in="$1"
>-    ip="$2"
>-    _maskbits_in="$3"
>-
>-    # Intentional word splitting here
>-    # shellcheck disable=SC2046
>-    set -- $(ip_maskbits_iface "$ip")
>-    if [ -n "$1" ] ; then
>-	maskbits="$1"
>-	iface="$2"
>-
>-	if [ "$iface" != "$_iface_in" ] ; then
>-	    printf \
>-		'WARNING: Public IP %s hosted on interface %s but VNN says %s\n' \
>-		"$ip" "$iface" "$_iface_in"
>-	fi
>-	if [ "$maskbits" != "$_maskbits_in" ] ; then
>-	    printf \
>-		'WARNING: Public IP %s has %s bit netmask but VNN says %s\n' \
>-		    "$ip" "$maskbits" "$_maskbits_in"
>+	_iface_in="$1"
>+	ip="$2"
>+	_maskbits_in="$3"
>+
>+	# Intentional word splitting here
>+	# shellcheck disable=SC2046
>+	set -- $(ip_maskbits_iface "$ip")
>+	if [ -n "$1" ]; then
>+		maskbits="$1"
>+		iface="$2"
>+
>+		if [ "$iface" != "$_iface_in" ]; then
>+			printf 'WARNING: Public IP %s hosted on interface %s but VNN says %s\n' \
>+				"$ip" "$iface" "$_iface_in"
>+		fi
>+		if [ "$maskbits" != "$_maskbits_in" ]; then
>+			printf 'WARNING: Public IP %s has %s bit netmask but VNN says %s\n' \
>+				"$ip" "$maskbits" "$_maskbits_in"
>+		fi
>+	else
>+		die "ERROR: Unable to determine interface for IP ${ip}"
> 	fi
>-    else
>-	die "ERROR: Unable to determine interface for IP ${ip}"
>-    fi
> }
> 
>-ip_block ()
>+ip_block()
> {
> 	_ip="$1"
> 	_iface="$2"
> 
> 	case "$_ip" in
> 	*:*) _family="inet6" ;;
>-	*)   _family="inet"  ;;
>+	*) _family="inet" ;;
> 	esac
> 
> 	# Extra delete copes with previously killed script
> 	iptables_wrapper "$_family" \
>-			 -D INPUT -i "$_iface" -d "$_ip" -j DROP 2>/dev/null
>+		-D INPUT -i "$_iface" -d "$_ip" -j DROP 2>/dev/null
> 	iptables_wrapper "$_family" \
>-			 -I INPUT -i "$_iface" -d "$_ip" -j DROP
>+		-I INPUT -i "$_iface" -d "$_ip" -j DROP
> }
> 
>-ip_unblock ()
>+ip_unblock()
> {
> 	_ip="$1"
> 	_iface="$2"
> 
> 	case "$_ip" in
> 	*:*) _family="inet6" ;;
>-	*)   _family="inet"  ;;
>+	*) _family="inet" ;;
> 	esac
> 
> 	iptables_wrapper "$_family" \
>-			 -D INPUT -i "$_iface" -d "$_ip" -j DROP 2>/dev/null
>+		-D INPUT -i "$_iface" -d "$_ip" -j DROP 2>/dev/null
> }
> 
> ctdb_check_args "$@"
>@@ -128,8 +126,8 @@ init)
> 	}
> 
> 	_promote="sys/net/ipv4/conf/all/promote_secondaries"
>-	get_proc "$_promote" >/dev/null 2>&1 || \
>-	    die "Public IPs only supported if promote_secondaries is available"
>+	get_proc "$_promote" >/dev/null 2>&1 ||
>+		die "Public IPs only supported if promote_secondaries is available"
> 
> 	# make sure we drop any ips that might still be held if
> 	# previous instance of ctdb got killed with -9 or similar
>@@ -152,7 +150,7 @@ takeip)
> 	update_my_public_ip_addresses "takeip" "$ip"
> 
> 	add_ip_to_iface "$iface" "$ip" "$maskbits" || {
>-		exit 1;
>+		exit 1
> 	}
> 
> 	# In case a previous "releaseip" for this IP was killed...
>@@ -213,7 +211,7 @@ updateip)
> 
> 	# Could check maskbits too.  However, that should never change
> 	# so we want to notice if it does.
>-	if [ "$oiface" = "$niface" ] ; then
>+	if [ "$oiface" = "$niface" ]; then
> 		echo "Redundant \"updateip\" - ${ip} already on ${niface}"
> 		exit 0
> 	fi
>-- 
>2.47.3
>
>
>From 457bc85c0c0b478a61920061984904f38675b210 Mon Sep 17 00:00:00 2001
>From: Martin Schwenke <mschwenke@ddn.com>
>Date: Thu, 16 Oct 2025 08:17:44 +1100
>Subject: [PATCH 2/5] ctdb-daemon: Fix a crash due to a failed updateip
>
>This should really be a takeip.  However, CTDB's weak check of the IP
>address state (using bind(2)) incorrectly indicates that the IP
>address is assigned to an interface so it is converted to an updateip.
>
>After commit 0536d7a98b832fc00d26b09c26bf14fb63dbf5fb (which improves
>IP address state checking), this will almost certainly not occur on
>platforms with getifaddrs(3) (e.g. Linux).  This means it is only
>likely to occur in 4.21 when net.ipv4.ip_nonlocal_bind=1.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15935
>
>Reported-by: Bailey Allison <ballison@45drives.com>
>Signed-off-by: Martin Schwenke <mschwenke@ddn.com>
>Reviewed-by: Anoop C S <anoopcs@samba.org>
>(cherry picked from commit d08f9ebd2755671d30c73a4e979029d353848828)
>---
> ctdb/server/ctdb_takeover.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
>diff --git a/ctdb/server/ctdb_takeover.c b/ctdb/server/ctdb_takeover.c
>index b9196e3ff63..f1b3119bf34 100644
>--- a/ctdb/server/ctdb_takeover.c
>+++ b/ctdb/server/ctdb_takeover.c
>@@ -613,7 +613,15 @@ static void ctdb_do_updateip_callback(struct ctdb_context *ctdb, int status,
> 		 */
> 		ctdb_vnn_unassign_iface(ctdb, state->vnn);
> 		state->vnn->iface = state->old;
>-		state->vnn->iface->references++;
>+		/*
>+		 * state->old (above) can be NULL if the IP wasn't
>+		 * recorded as held by this node but the system thinks
>+		 * the IP was assigned.  In that case, a move could
>+		 * still be desirable..
>+		 */
>+		if (state->vnn->iface != NULL) {
>+			state->vnn->iface->references++;
>+		}
> 
> 		ctdb_request_control_reply(ctdb, state->c, NULL, status, NULL);
> 		talloc_free(state);
>-- 
>2.47.3
>
>
>From 39c5317e984182c1160f40fca47d1edcf1883da5 Mon Sep 17 00:00:00 2001
>From: Martin Schwenke <mschwenke@ddn.com>
>Date: Thu, 16 Oct 2025 10:42:22 +1100
>Subject: [PATCH 3/5] ctdb-tests: Add an event script unit test for updateip
>
>This illustrates the current failure where an unassigned public IP
>address causes updateip to fail.
>
>After commit 0536d7a98b832fc00d26b09c26bf14fb63dbf5fb (which improves
>IP address state checking), this will almost certainly not occur on
>platforms with getifaddrs(3) (e.g. Linux).  This means it is only
>likely to occur in 4.21 when net.ipv4.ip_nonlocal_bind=1.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15935
>
>Reported-by: Bailey Allison <ballison@45drives.com>
>Signed-off-by: Martin Schwenke <mschwenke@ddn.com>
>Reviewed-by: Anoop C S <anoopcs@samba.org>
>(cherry picked from commit a98ffb96efc4a9ea2110c654860a4ba3896ab3d5)
>---
> .../eventscripts/10.interface.updateip.001.sh    | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
> create mode 100755 ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh
>
>diff --git a/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh b/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh
>new file mode 100755
>index 00000000000..af87bc14326
>--- /dev/null
>+++ b/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh
>@@ -0,0 +1,16 @@
>+#!/bin/sh
>+
>+. "${TEST_SCRIPTS_DIR}/unit.sh"
>+
>+define_test "error - update a non-existent ip"
>+
>+setup
>+
>+public_address=$(ctdb_get_1_public_address)
>+ip="${public_address% *}"
>+ip="${ip#* }"
>+
>+required_result 1 "ERROR: Unable to determine interface for IP ${ip}"
>+# Want separate words from public_address: interface IP maskbits
>+# shellcheck disable=SC2086
>+simple_test "__none__" $public_address
>-- 
>2.47.3
>
>
>From 061d017d7739ab1839e83bc70a47ec6e1180433b Mon Sep 17 00:00:00 2001
>From: Martin Schwenke <mschwenke@ddn.com>
>Date: Thu, 16 Oct 2025 13:51:27 +1100
>Subject: [PATCH 4/5] ctdb-scripts: Avoid printing a message if no connections
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15935
>
>Signed-off-by: Martin Schwenke <mschwenke@ddn.com>
>Reviewed-by: Anoop C S <anoopcs@samba.org>
>(cherry picked from commit 01d3d25c0139a3dd49a2322a9416698d08733377)
>---
> ctdb/config/functions | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/ctdb/config/functions b/ctdb/config/functions
>index 4139059a3d3..d61852a8161 100755
>--- a/ctdb/config/functions
>+++ b/ctdb/config/functions
>@@ -594,6 +594,10 @@ tickle_tcp_connections()
> 	_conns=$(get_tcp_connections_for_ip "$_ip" |
> 		awk '{ print $1, $2 ; print $2, $1 }')
> 
>+	if [ -z "$_conns" ]; then
>+		return
>+	fi
>+
> 	echo "$_conns" | awk '{ print "Tickle TCP connection", $1, $2 }'
> 	echo "$_conns" | ctdb tickle
> }
>-- 
>2.47.3
>
>
>From 445f37243dee3fa905744ca93f1c5362d1286d66 Mon Sep 17 00:00:00 2001
>From: Martin Schwenke <mschwenke@ddn.com>
>Date: Thu, 16 Oct 2025 13:54:22 +1100
>Subject: [PATCH 5/5] ctdb-scripts: Avoid failing updateip when IP is not
> assigned
>
>There is no use failing this when it could behave more like takeip.
>
>Use old interface of "__none__" as a hint that ctdbd doesn't think the
>IP is assigned either.  In this case print a warning instead of an
>error.  Take some care to avoid spurious errors in updateip.
>
>After commit 0536d7a98b832fc00d26b09c26bf14fb63dbf5fb (which improves
>IP address state checking), this will almost certainly not occur on
>platforms with getifaddrs(3) (e.g. Linux).  This means it is only
>likely to occur in 4.21 when net.ipv4.ip_nonlocal_bind=1.
>
>Update test to match.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15935
>
>Reported-by: Bailey Allison <ballison@45drives.com>
>Signed-off-by: Martin Schwenke <mschwenke@ddn.com>
>Reviewed-by: Anoop C S <anoopcs@samba.org>
>
>Autobuild-User(master): Anoop C S <anoopcs@samba.org>
>Autobuild-Date(master): Fri Oct 17 06:28:30 UTC 2025 on atb-devel-224
>
>(cherry picked from commit 0e73781bf84a1e8e596d8be3f55eeb5f8f927990)
>---
> ctdb/config/events/legacy/10.interface.script   | 17 +++++++++++++----
> .../eventscripts/10.interface.updateip.001.sh   |  2 +-
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
>diff --git a/ctdb/config/events/legacy/10.interface.script b/ctdb/config/events/legacy/10.interface.script
>index 137d558b3b7..f0545a40455 100755
>--- a/ctdb/config/events/legacy/10.interface.script
>+++ b/ctdb/config/events/legacy/10.interface.script
>@@ -80,6 +80,11 @@ get_iface_ip_maskbits()
> 				"$ip" "$maskbits" "$_maskbits_in"
> 		fi
> 	else
>+		if [ "$_iface_in" = "__none__" ]; then
>+			echo "WARNING: Unable to determine interface for IP ${ip}"
>+			iface="$_iface_in"
>+			return
>+		fi
> 		die "ERROR: Unable to determine interface for IP ${ip}"
> 	fi
> }
>@@ -216,10 +221,14 @@ updateip)
> 		exit 0
> 	fi
> 
>-	ip_block "$ip" "$oiface"
>-
>-	delete_ip_from_iface "$oiface" "$ip" "$maskbits" 2>/dev/null
>-	delete_ip_from_iface "$niface" "$ip" "$maskbits" 2>/dev/null
>+	# Behave more like takeip when the IP is not assigned.  No
>+	# need for a similar condition around ip_unblock()s because
>+	# they will silently fail.
>+	if [ "$oiface" != "__none__" ]; then
>+		ip_block "$ip" "$oiface"
>+		delete_ip_from_iface "$oiface" "$ip" "$maskbits" >/dev/null 2>&1
>+	fi
>+	delete_ip_from_iface "$niface" "$ip" "$maskbits" >/dev/null 2>&1
> 
> 	add_ip_to_iface "$niface" "$ip" "$maskbits" || {
> 		ip_unblock "$ip" "$oiface"
>diff --git a/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh b/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh
>index af87bc14326..e9567a8d114 100755
>--- a/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh
>+++ b/ctdb/tests/UNIT/eventscripts/10.interface.updateip.001.sh
>@@ -10,7 +10,7 @@ public_address=$(ctdb_get_1_public_address)
> ip="${public_address% *}"
> ip="${ip#* }"
> 
>-required_result 1 "ERROR: Unable to determine interface for IP ${ip}"
>+ok "WARNING: Unable to determine interface for IP ${ip}"
> # Want separate words from public_address: interface IP maskbits
> # shellcheck disable=SC2086
> simple_test "__none__" $public_address
>-- 
>2.47.3
>

Flags:

anoopcs: review+

Actions: View

Attachments on bug 15935: 18760 | 18761 | 18762