Forum OpenACS Q&A: Issue with reachable method in 5.10.1 dynamic clustering setup

Our member nodes join with addresses in the format of https://10.x.y.z (no port).

You can see in the log messages here the join request

[14/Nov/2023:11:04:32][31569.7f117cce3700][-conn:default:0:3269-] Notice: --cluster executes command '::acs::cluster join_request https://10.34.24.222'; from peeraddr 10.34.24.222 port 58181
[14/Nov/2023:11:04:32][31569.7f117cce3700][-conn:default:0:3269-] Notice: Cluster join_request from 'https://10.34.24.222';
[14/Nov/2023:11:04:32][31569.7f117cce3700][-conn:default:0:3269-] Notice: Cluster join_request https://10.34.24.222 accepted from https://10.34.24.222
[14/Nov/2023:11:04:32][31569.7f117cce3700][-conn:0:3269-] Notice: Cluster join_request leads to DynamicClusterPeers https://10.34.24.222

The issue occurs within the reachability check inside packages/acs-tcl/tcl/cluster-procs.tcl (This is in acs-tcl package version 5.10.1b1)

The code parses the url as seen below:

:method reachable {location} {
            #:log "reachable $location"
            set d [ns_parseurl $location]

and then further down attempts to connect with `ns_connchan connect $host $port`

However given our nodes are registering without a port there is no $port variable which causes an exception. This exception causes the member node to be considered 'not reachable'

A patch for my own environment is to add the following code:

                        try {
                            if {![info exists port]} {
                                if {$proto eq "http"} {
                                    set port 80
                                } else {
                                    set port 443
                                }
                            }
                            #ns_logctl severity Debug(connchan) on
                            ns_connchan connect $host $port
                        } on error {} {

I assume maybe it is always expecting it to be registering nodes with <proto>://<host>:<port> but in our environment that is not how the nodes are registering to be listed in dynamic cluster peers.

Here is some logging I added to the current_server_locations method to debug

[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations: use module <nsssl> /web/dev/node-1/httpd/bin/nsssl.so
[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations: ip 10.34.203.163
[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations: port 443
[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations: use module <nssock> /web/dev/node-1/httpd/bin/nssock.so
[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations: ip 10.34.203.163
[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations: port 80
[21/Nov/2023:09:29:25][21757.7f4344aeb840][-main:-1-] Notice: current_server_locations returns http://10.34.203.163 https://10.34.203.163

Here is the relevant code that results in the port being dropped:

packages/acs-tcl/tcl/cluster-procs.tcl

lappend result [util::join_location \
      -proto [dict get $protos $module_type] \
      -hostname $ip \
      -port $port]

packages/acs-tcl/tcl/utilities-procs.tcl

ad_proc util::join_location {{-proto ""} {-hostname} {-port ""}} {
    Join hostname and port and use IP-literal notation when necessary.
    The function is the inverse function of  util::split_location.
    @return location consisting of hostname and optionally port
    @author Gustaf Neumann
    @see util::split_location
} {
    set result ""
    if {$proto ne ""} {
        append result $proto://
        #
        # When the specified port is equal to the default port, omit
        # it from the result.
        #
        if {$port ne "" && $port eq [dict get {http 80 https 443 udp "" smtp ""} $proto]} {
            set port ""
        }
    }

I tried this change out but the method calls util::join_location which has the problem I pointed out before that drops the standard port.

https://cvs.openacs.org/browse/OpenACS/openacs-4/packages/acs-tcl/tcl/cluster-procs.tcl?r=1.1.2.9#to524

This causes the same problem with nodes being considered not reachable due to the missing port

[27/Nov/2023:11:37:02][27994.7f62556f3700][-conn:default:0:10-] Notice: Cluster join_request from 'https://10.34.203.85';
[27/Nov/2023:11:37:02][27994.7f62556f3700][-conn:default:0:10-] Notice: Cluster join_request https://10.34.203.85 accepted from https://10.34.203.85
[27/Nov/2023:11:37:02][27994.7f62556f3700][-conn::default:0:10-] Notice: Cluster join_request leads to DynamicClusterPeers https://10.34.203.85
[27/Nov/2023:11:37:05][27994.7f624d7fa700][-sched:0:8:3-] Notice: Running scheduled proc ::acs::cluster {*}update_node_info...
[27/Nov/2023:11:37:05][27994.7f624d7fa700][-sched:0:8:3-] Notice: cluster: node https://10.34.203.85 is reachable: 0 last_contact 1701113824362 last_request 1701113824362

Best,
Jon

I tested out the changes you have made and found that a few more changes are required and one slight change to the util::join_location method.

Here is a git diff showing the necessary changes. Let me know if you need more info since maybe the line numbers don't match up on your side. It includes all of the changes from this thread and a couple of other changes. Mainly two additional changes

1. Inside current_server_locations needs to use the noabbrev flag as well otherwise it won't match when checking against the dynamic peers list to see if it is already in the list and a few other checks

2. In util::join_location the port was being set to "" before the new if clause in your last patch since $proto is set. Needed to make sure noabbrev_p  is false in that case to not clear the port. Also the new check needs to validate noabbrev_p is true rather than false to append the port.

diff --git a/packages/acs-tcl/tcl/cluster-procs.tcl b/packages/acs-tcl/tcl/cluster-procs.tcl
@@ -529,7 +530,7 @@ namespace eval ::acs {
            #
            dict set d host [ns_addrbyhost [dict get $d host]]
            dict with d {
-                set result [util::join_location -proto $proto -hostname $host -port $port]
+                set result [util::join_location -noabbrev -proto $proto -hostname $host -port $port]
            }
            return $result
        }
@@ -586,15 +587,17 @@ namespace eval ::acs {
                                lappend result [util::join_location \
-                                                    -proto [dict get $protos $module_type] \
+                                                    -noabbrev -proto [dict get $protos $module_type] \
                                                    -hostname $ip \
                                                    -port $port]
                            }
@@ -646,7 +649,8 @@ namespace eval ::acs {
                #
                ns_log notice "Cluster join_request $peerLocation accepted from $peerLocation"
                set dynamicClusterNodes [parameter::get -package_id $::acs::kernel_id -parameter DynamicClusterPeers]
-                set dynamicClusterNodes [lsort -unique [concat $dynamicClusterNodes $peerLocation]]
+                set dynamicClusterNodes [lsort -unique [concat $dynamicClusterNodes [:qualified_location $peerLocation]]]
                #
                # The parameter::set_value operation causes a
                # clusterwide cache-flush for the parameters

diff --git a/packages/acs-tcl/tcl/utilities-procs.tcl b/packages/acs-tcl/tcl/utilities-procs.tcl
index 4b41b73ba..89b289b22 100644
--- a/packages/acs-tcl/tcl/utilities-procs.tcl
+++ b/packages/acs-tcl/tcl/utilities-procs.tcl
@@ -1923,13 +1923,16 @@ ad_proc util::split_location {location protoVar hostnameVar portVar} {
    return $success
}

-ad_proc util::join_location {{-proto ""} {-hostname} {-port ""}} {
+ad_proc util::join_location {{-noabbrev:boolean} {-proto ""} {-hostname} {-port ""}} {
    Join hostname and port and use IP-literal notation when necessary.
    The function is the inverse function of  util::split_location.
+    @param noabbrev when specified, the location is joined as requested.
+                     Otherwise, default ports are omitted from the result.
    @return location consisting of hostname and optionally port
    @author Gustaf Neumann
    @see util::split_location
} {
    set result ""
    if {$proto ne ""} {
        append result $proto://
@@ -1937,7 +1940,7 @@ ad_proc util::join_location {{-proto ""} {-hostname} {-port ""}} {
        # When the specified port is equal to the default port, omit
        # it from the result.
        #
-        if {$port ne "" && $port eq [dict get {http 80 https 443 udp "" smtp ""} $proto]} {
+        if {!$noabbrev_p && $port ne "" && $port eq [dict get {http 80 https 443 udp "" smtp ""} $proto]} {
            set port ""
        }
    }
@@ -1946,7 +1949,10 @@ ad_proc util::join_location {{-proto ""} {-hostname} {-port ""}} {
    } else {
        append result $hostname
    }
-    if {$port ne ""} {
+    if {$noabbrev_p
+         && $port ne ""
+         && $port eq [dict get {http 80 https 443 udp "" smtp ""} $proto]
+     } {
        append result :$port
    }
    return $result

We have an architecture that would scale up and down based on load and this list of dynamic peers would be cluttered if we didn't do some manual pruning of the parameter.

It looks like it would require manual cleanup using the status page as a guide for the level of inactivity?

I haven't pulled down the latest but I didn't see any code change in your commits or the source I have that does this cleanup.

If there is code to do the automatic cleanup can you send along a pointer so I can review.

Thanks.

We have some logic to clean up similar parameters ClusterPeerIP and ClusterAuthorizedIP for this type of situation but thought maybe the DynamicClusterPeers would be self-controlled within the core functionality.

Thanks.