Fix connection accounting for revcons

Changes from review - Tx Akshat

Signed-off-by: Mark Hannum <mhannum@bloomberg.net>

Author: markhannum

db/db_tunables.c

@@ -541,6 +541,7 @@ extern int gbl_revsql_force_rte;
 extern int gbl_revsql_fake_connect_failure;
 extern int gbl_connect_remote_rte;
 extern int gbl_reverse_hosts_v2;
+extern int gbl_admin_revsql;
 
 int gbl_debug_tmptbl_corrupt_mem;
 int gbl_group_concat_mem_limit; /* 0 implies allow upto SQLITE_MAX_LENGTH,

db/db_tunables.h

@@ -1940,6 +1940,8 @@ REGISTER_TUNABLE("revsql_host_refresh_freq_sec", "The frequency at which the "
                  "reverse connection host list will be refreshed (Default: 5secs)",
                  TUNABLE_INTEGER, &gbl_revsql_host_refresh_freq_sec, EXPERIMENTAL | INTERNAL,
                  NULL, NULL, NULL, NULL);
+REGISTER_TUNABLE("admin_revsql", "Run revsql sessions as admin.  (Default: Off)", TUNABLE_BOOLEAN, &gbl_admin_revsql, 0,
+                 NULL, NULL, NULL, NULL);
 REGISTER_TUNABLE("reverse_hosts_v2", "Use reverse_hosts_v2, which includes class and cluster.  (Default: Off)",
                  TUNABLE_BOOLEAN, &gbl_reverse_hosts_v2, EXPERIMENTAL | INTERNAL, NULL, NULL, NULL, NULL);
 REGISTER_TUNABLE("gbl_class_machs_refresh", "Requery-time for class-machine lookup.  (Default: 300s)", TUNABLE_INTEGER,

db/reverse_conn.c

@@ -91,10 +91,12 @@ enum {
 };
 
 int gbl_revsql_force_rte = 1;
+int gbl_admin_revsql = 0;
 
 int send_reversesql_request(const char *dbname, const char *host, const char *command)
 {
     int rc = 0;
+    int isadmin = gbl_admin_revsql ? 1 : 0;
 
     if (gbl_revsql_debug == 1) {
         revconn_logmsg(LOGMSG_USER, "%s:%d Sending reversesql request to %s@%s\n", __func__, __LINE__, dbname, host);
@@ -109,6 +111,11 @@ int send_reversesql_request(const char *dbname, const char *host, const char *co
     int new_fd = sbuf2fileno(sb);
     make_server_socket(new_fd);
 
+    if (revcon_should_reject(isadmin)) {
+        rc = -1;
+        goto cleanup;
+    }
+
     if (db_is_exiting()) {
         if (gbl_revsql_debug == 1) {
             revconn_logmsg(LOGMSG_USER, "%s:%d Comdb2 is exiting\n", __func__, __LINE__);

net/net_evbuffer.c

@@ -2930,6 +2930,17 @@ static int should_reject_request(uint8_t first_byte)
     return check_appsock_limit(pending_connections, is_admin);
 }
 
+struct should_reject_request_arg {
+    int ret;
+    int isadmin;
+};
+
+static void invoke_should_reject_request(void *data)
+{
+    struct should_reject_request_arg *arg = data;
+    arg->ret = should_reject_request(arg->isadmin ? '@' : 'n');
+}
+
 /* PMUV REQUEST/RESPONSE */
 enum request { V_WHO = 1, V_NAK = 2, V_ACK = 3 };
 enum response { V_NONE = 0, V_ID = 1 };
@@ -3978,6 +3989,13 @@ int get_hosts_metric(const char *netname, enum net_metric_type type)
     return metric.value;
 }
 
+int revcon_should_reject(int admin)
+{
+    struct should_reject_request_arg arg = {.isadmin = admin};
+    run_on_base(base, invoke_should_reject_request, &arg);
+    return arg.ret;
+}
+
 int add_appsock_handler(const char *key, event_callback_fn cb)
 {
     size_t keylen = strlen(key);

net/net_int.h

@@ -359,6 +359,7 @@ enum net_metric_type {
     MAX_QUEUE_SIZE = 2
 };
 int get_hosts_metric(const char *netname, enum net_metric_type type);
+int revcon_should_reject(int admin);
 
 int dist_heartbeats(dist_hbeats_type *);
 

tests/phys_rep_tiered.test/runit

@@ -1796,6 +1796,21 @@ function phys_rep_nomatch
     return 0
 }
 
+# rev-cons were decrementing active_appsock_conns without incrementing
+# Verify that there are a sane number of 'concurrent_connections' in comdb2_metrics
+function verify_connection_count
+{
+    if [[ -z "$CLUSTER" ]]; then
+        num=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $DBNAME default "select value from comdb2_metrics where name='concurrent_connections'")
+        [[ "$num" == -* ]] && cleanFailExit "Connection count is negative"
+    else
+        for node in ${CLUSTER} ; do
+            num=$($CDB2SQL_EXE --tabs ${CDB2_OPTIONS} $DBNAME --host $node "select value from comdb2_metrics where name='concurrent_connections'")
+            [[ "$num" == -* ]] && cleanFailExit "Connection count is negative"
+        done
+    fi
+}
+
 trap - INT EXIT
 
 # 1. Setup replication metadata cluster
@@ -2016,6 +2031,11 @@ function run_tests
     testcase_preamble $testcase
     check_metadb_memstat
     testcase_finish $testcase
+
+    testcase="verify_connection_count"
+    testcase_preamble $testcase
+    verify_connection_count
+    testcase_finish $testcase
 }
 
 run_tests

tests/tunables.test/t00_all_tunables.expected

@@ -21,6 +21,7 @@
 (name='add_record_interval', description='Add a record every seconds while there are incoherent_wait replicants.', type='INTEGER', value='1', read_only='N')
 (name='additional_deferms', description='Wait-fudge to ensure that a replicant has gone incoherent.', type='INTEGER', value='0', read_only='N')
 (name='admin_mode', description='Fail non-admin client requests (Default: False)', type='BOOLEAN', value='OFF', read_only='N')
+(name='admin_revsql', description='Run revsql sessions as admin.  (Default: Off)', type='BOOLEAN', value='OFF', read_only='N')
 (name='all_incoherent', description='Master pretends nodes are incoherent.', type='BOOLEAN', value='OFF', read_only='N')
 (name='allow_anon_id_for_spmux', description='Allow anonymous identities over connections routed from the secure pmux port', type='INTEGER', value='0', read_only='N')
 (name='allow_broken_datetimes', description='Allow broken datetimes', type='BOOLEAN', value='ON', read_only='N')