CDRIVER-3625 monitor with a thread-per-server #607

kevinAlbs · 2020-05-07T18:11:32Z

Prerequisite work for implementing streamable ismaster in multi-threaded.
Background and rationale is described in WRITING-5411.

For a client pool, servers are now scanned independently using a separate thread per server.
SRV polling is now done on a separate thread.

For a client pool, servers are now scanned independently using a separate thread per server. SRV polling is now done on a separate thread.

kevinAlbs · 2020-05-07T18:14:06Z

src/libmongoc/src/mongoc/mongoc-topology.c

         rr_data.min_ttl * 1000, MONGOC_TOPOLOGY_MIN_RESCAN_SRV_INTERVAL_MS);

      topology_valid = true;
   srv_fail:
      bson_free (rr_data.txt_record_opts);
      bson_free (prefixed_service);
+      _mongoc_host_list_destroy_all (rr_data.hosts);


This leak appeared when rerunning the manual test for SRV polling.

kevinAlbs · 2020-05-07T18:56:43Z

.evergreen/compile-unix.sh

@@ -262,10 +262,10 @@ mkfifo pipe || true
 if [ -e pipe ]; then
   set +o xtrace
   tee error.log < pipe &
-   run_valgrind ./src/libmongoc/test-libmongoc -d -F test-results.json 2>pipe
+   run_valgrind ./src/libmongoc/test-libmongoc --no-fork -d -F test-results.json 2>pipe


This lets debug logs to print to stdout. There's likely a better way to
accomplish that. But the only downside of running with --no-fork I'm aware of is
that the first test failure fails the entire process. That did not seem like a
big loss to me. Though, I'm happy to change back and look for alternatives if
there's disagreement

kevinAlbs · 2020-05-07T18:57:08Z

.evergreen/run-tests.sh

@@ -93,7 +93,7 @@ case "$OS" in
      check_mongocryptd

      chmod +x src/libmongoc/Debug/test-libmongoc.exe
-      ./src/libmongoc/Debug/test-libmongoc.exe $TEST_ARGS
+      ./src/libmongoc/Debug/test-libmongoc.exe $TEST_ARGS -d


I tried putting --no-fork here, but then stumbled on an existing bug: https://jira.mongodb.org/browse/CDRIVER-3637

kevinAlbs · 2020-05-07T18:58:31Z

src/libmongoc/src/mongoc/mongoc-client.c

-                                        const mongoc_host_list_t *host,
-                                        void *user_data,
-                                        bson_error_t *error)
+mongoc_client_connect (bool buffered,


The server monitors do not have a mongoc_client_t, but need to create a stream to a server given just a host address.

mongoc_client_connect establishes connection with just the address and SSL options, and is used by mongoc_client_default_stream_initiator, which takes the SSL options from a mongoc_client_t.

kevinAlbs · 2020-05-07T18:58:50Z

src/libmongoc/src/mongoc/mongoc-client.c

-                                        bson_error_t *error)
+mongoc_client_connect (bool buffered,
+                       bool use_ssl,
+                       void *ssl_opts_void,


Sidenote: I found it frustrating to work with mongoc_ssl_opt_t. Since it is conditionally defined, using it requires either hiding the type or using typedefs.

kevinAlbs · 2020-05-07T18:59:38Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+
+/* Called only from server monitor thread.
+ * Caller must hold no locks (user's callback may lock topology mutex).
+ * Locks APM mutex.


I had not realized until later, but by scanning servers independently, if we did not lock, this would change the thread-safety requirements of the callbacks.

In the previous behavior, since all monitoring happened in one thread, only one SDAM callback would be called at a time.

To persist that behavior, I've added a separate mutex. This cannot use the topology mutex, since the user's callback may need to obtain the topology mutex (e.g. if they do an operation that requests a scan, does server selection, etc.)

kevinAlbs · 2020-05-07T19:00:58Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+}
+
+static bool
+_server_monitor_cmd_send (mongoc_server_monitor_t *server_monitor,


This is closely modelled from mongoc-async-cmd.c, and will be modified to support streamable ismaster (will need to send OP_MSG exhaust cursors if the server monitor transitions to streaming).

kevinAlbs · 2020-05-07T19:01:56Z

src/libmongoc/src/mongoc/mongoc-topology.c

+ * servers that were removed from the topology.
+ */
+static void
+_topology_collect_errors (mongoc_topology_t *topology, bson_error_t *error_out)


The single-threaded mongoc_topology_scanner_t keeps the last seen error per server scanned. This could include an error received on a server that was removed from the topology if it had been scanned during the full blocking scan.
But in multi-threaded, servers are scanned independently, there's no clear way to track errors of servers that were removed (how long would we keep those errors around for?).

Instead, take errors from the server descriptions in the topology. This aligns with the server selection spec's recommendation of how to return errors:
https://github.com/mongodb/specifications/blob/master/source/server-selection/server-selection.rst#server-selection-errors

kevinAlbs · 2020-05-07T19:02:11Z

src/libmongoc/tests/test-libmongoc.c

@@ -412,6 +412,9 @@ log_handler (mongoc_log_level_t log_level,
      if (!suite->silent) {
         mongoc_log_default_handler (log_level, log_domain, message, NULL);
      }
+   } else if (log_level == MONGOC_LOG_LEVEL_DEBUG &&


Now, if -d is passed to test-libmongoc, MONGOC_DEBUG messages will be printed.

kevinAlbs · 2020-05-07T19:02:47Z

src/libmongoc/tests/test-mongoc-topology-reconcile.c

@@ -140,8 +137,10 @@ _test_topology_reconcile_rs (bool pooled)
    * server0 is selected, server1 is discovered and added to scanner.
    */
   BSON_ASSERT (selects_server (client, secondary_read_prefs, server0));
-   BSON_ASSERT (
-      get_node (client->topology, mock_server_get_host_and_port (server1)));
+   if (!pooled) {


Some tests reach inside the mongoc_topology_scanner_t to make assertions. This is only applicable for single threaded now.

kevinAlbs · 2020-05-07T19:03:21Z

src/libmongoc/tests/test-mongoc-topology-reconcile.c

@@ -456,13 +447,12 @@ test_topology_reconcile_from_handshake (void *ctx)
 * mode for good measure.
 */
 static void
-_test_topology_reconcile_retire (bool pooled)
+test_topology_reconcile_retire_single (void)


The single threaded scanner has a concept of "retired" nodes, for nodes removed in the middle of a full scan. There is no equivalent for multi-threaded, so this test is now limited to single-threaded.

kevinAlbs · 2020-05-07T19:05:02Z

src/libmongoc/tests/test-mongoc-topology.c

+   int n_succeeded;
+   int n_failed;
+   int n_unknowns;
+   bson_mutex_t mutex;


These counters are modified in SDAM callbacks and checked by the test. It wasn't thread-safe before and was showing up when running TSAN locally. Now, updating and reading them happens behind a mutex.

kevinAlbs · 2020-05-07T19:06:12Z

src/libmongoc/tests/test-mongoc-topology.c

@@ -1684,38 +1784,6 @@ test_cluster_time_updated_during_handshake ()
   mongoc_uri_destroy (uri);
 }

-/* returns the last time the topology completed a full scan. */
-static int64_t
-_get_last_scan (mongoc_client_t *client)


This test required some changing. _get_last_scan is no longer a thing for multi-threaded since servers are scanned independently.

kevinAlbs · 2020-05-07T19:06:45Z

src/libmongoc/tests/test-mongoc-topology.c

@@ -1433,28 +1529,32 @@ _test_ismaster_retry_pooled (bool hangup, int n_failures)
   /* retry immediately (for testing, "immediately" means less than 250ms */
   request = mock_server_receives_ismaster (server);
   ASSERT_CMPINT64 (bson_get_monotonic_time () - t, <, (int64_t) 250 * 1000);
+   /* Since connection was established successfully, the server description is
+    * not marked as Unknown until after a failed retry attempt. */
+   BSON_ASSERT (has_known_server (client));


The previous behavior was incorrect, a failure would mark as Unknown before attempting a retry: https://jira.mongodb.org/browse/CDRIVER-3529

alcaeus

From what I can tell, functionality looks good. I couldn't resist nitpicking, but I'll leave the decision on coding style to you.

alcaeus · 2020-05-19T12:21:01Z

src/libmongoc/src/mongoc/mongoc-topology.c

+/* Called from application threads
+ * Caller must hold topology lock.
+ * Locks topology description mutex to copy out server description errors.
+ * CLEANUP: this has nothing to do with background monitoring. Move to


Adding a note here: did you want to clean this up in this PR or in a follow-up?

Ah, I moved it, but forgot to update the comment. Fixed.

alcaeus · 2020-05-19T12:28:34Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+   if (server_monitor->apm_callbacks.server_heartbeat_started) {
+      mongoc_apm_server_heartbeat_started_t event;
+      event.host = &server_monitor->host;
+      event.context = server_monitor->apm_context;
+      bson_mutex_lock (&server_monitor->topology->apm_mutex);
+      server_monitor->apm_callbacks.server_heartbeat_started (&event);
+      bson_mutex_unlock (&server_monitor->topology->apm_mutex);
+   }


Nit: using a guard clause helps reduce nesting, but I'm aware that most code here wraps logic in the condition instead of exiting early.

Suggested change

if (server_monitor->apm_callbacks.server_heartbeat_started) {

mongoc_apm_server_heartbeat_started_t event;

event.host = &server_monitor->host;

event.context = server_monitor->apm_context;

bson_mutex_lock (&server_monitor->topology->apm_mutex);

server_monitor->apm_callbacks.server_heartbeat_started (&event);

bson_mutex_unlock (&server_monitor->topology->apm_mutex);

}

if (!server_monitor->apm_callbacks.server_heartbeat_started) {

return;

}

mongoc_apm_server_heartbeat_started_t event;

event.host = &server_monitor->host;

event.context = server_monitor->apm_context;

bson_mutex_lock (&server_monitor->topology->apm_mutex);

server_monitor->apm_callbacks.server_heartbeat_started (&event);

bson_mutex_unlock (&server_monitor->topology->apm_mutex);

Good idea, done.

alcaeus · 2020-05-19T12:29:36Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+                                     const bson_t *reply,
+                                     int64_t duration_usec)
+{
+   if (server_monitor->apm_callbacks.server_heartbeat_succeeded) {


Nit above applies here as well.

alcaeus · 2020-05-19T12:29:40Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+                                  const bson_error_t *error,
+                                  int64_t duration_usec)
+{
+   if (server_monitor->apm_callbacks.server_heartbeat_failed) {


Nit above applies here as well.

alcaeus · 2020-05-19T12:36:40Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+      topology->min_heartbeat_frequency_msec;
+   server_monitor->connect_timeout_ms = topology->connect_timeout_msec;
+   server_monitor->uri = mongoc_uri_copy (topology->uri);
+/* TODO: Do not retrieve ssl opts from topology scanner. They should be stored


Is there a tracking ticket to make this change to were you planning to do that in this PR?

Good point, created https://jira.mongodb.org/browse/CDRIVER-3682 to track this and updated the comment.

alcaeus · 2020-05-19T12:38:45Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+   if (topology->scanner_state == MONGOC_TOPOLOGY_SCANNER_BG_RUNNING) {
+      topology->scanner_state = MONGOC_TOPOLOGY_SCANNER_SHUTTING_DOWN;
+   } else {
+      return;
+   }


Nit for readability:

Suggested change

if (topology->scanner_state == MONGOC_TOPOLOGY_SCANNER_BG_RUNNING) {

topology->scanner_state = MONGOC_TOPOLOGY_SCANNER_SHUTTING_DOWN;

} else {

return;

}

if (topology->scanner_state != MONGOC_TOPOLOGY_SCANNER_BG_RUNNING) {

return;

}

topology->scanner_state = MONGOC_TOPOLOGY_SCANNER_SHUTTING_DOWN;

kevinAlbs · 2020-05-21T21:30:53Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+                         MONGOC_ERROR_PROTOCOL,
+                         MONGOC_ERROR_PROTOCOL_INVALID_REPLY,
+                         "Could not decompress server reply");
+         return MONGOC_ASYNC_CMD_ERROR;


Wrong return. Will fix in next commit.

bazile-clyde

Just a few comments but otherwise LGTM. There's one method I commented on that doesn't appear to be touched by any of the testings though.

bazile-clyde · 2020-05-27T18:56:37Z

src/libmongoc/src/mongoc/mongoc-trace-private.h

@@ -114,7 +114,7 @@ BSON_BEGIN_DECLS
                  BSON_FUNC,                                     \
                  __LINE__,                                      \
                  #_n,                                           \
-                  _iov,                                          \
+                  (void *) _iov,                                 \


Was this done for warnings?

Yes I was getting compiler warnings when compiling with tracing.

bazile-clyde · 2020-05-27T19:01:27Z

src/libmongoc/tests/mock_server/mock-server.h

-#define RS_RESPONSE_TO_ISMASTER(server, primary, has_tags, ...) \
-   rs_response_to_ismaster (server, primary, has_tags, __VA_ARGS__, NULL)
+#define RS_RESPONSE_TO_ISMASTER(                     \
+   server, max_wire_version, primary, has_tags, ...) \


Was max_wire_version being named primary a mistake? I don't really see why it was named that and not this in the first place.

Yeah, primary was misnamed, and was really being used as the max_wire_version. And the has_tags boolean was being passed as the primary boolean.

bazile-clyde · 2020-05-28T16:50:32Z

src/libmongoc/tests/test-mongoc-background-monitoring.c

+   tf = tf_new (0);
+
+   for (i = 1; i < 100; i++) {
+      MONGOC_DEBUG ("i=%d", i);


Did you mean to leave this here?

Ah, I did not. Removed.

bazile-clyde · 2020-05-29T14:41:03Z

src/libmongoc/src/mongoc/mongoc-topology-background-monitoring.c

+#undef MONGOC_LOG_DOMAIN
+#define MONGOC_LOG_DOMAIN "bg_monitor"
+
+static BSON_THREAD_FUN (srv_polling_run, topology_void)


Are there any tests for this method?

Unfortunately, there are no automated tests. The SRV polling spec requires manual testing: https://github.com/mongodb/specifications/tree/master/source/polling-srv-records-for-mongos-discovery/tests which I ran through following these instructions: https://github.com/kevinAlbs/SRVPollingTesting. This did flush out some bugs. E.g. originally the polling function locked the topology mutex during the DNS call. This would block server selection and monitoring.

Unit testing this seemed out of scope for now. I think we'd need to somehow mock the DNS server, or introduce new test seams. But this could be additional motivation for introducing new testing infrastructure. I created https://jira.mongodb.org/browse/CDRIVER-3693 to track that.

Sidenote: there is a DRIVERS ticket for improving the SRV polling integration tests, https://jira.mongodb.org/browse/DRIVERS-1025

kevinAlbs · 2020-05-29T22:37:29Z

Fixed merge conflicts locally and pushed in 370fc18 (and cherry-picked to r1.17 in c62098b).

CDRIVER-3625 monitor with a thread-per-server

983e015

For a client pool, servers are now scanned independently using a separate thread per server. SRV polling is now done on a separate thread.

kevinAlbs commented May 7, 2020

View reviewed changes

kevinAlbs changed the title ~~[WIP] CDRIVER-3625 monitor with a thread-per-server~~ CDRIVER-3625 monitor with a thread-per-server May 7, 2020

kevinAlbs requested a review from bazile-clyde May 7, 2020 19:10

add tests, guided by code coverage

0302ef1

kevinAlbs mentioned this pull request May 12, 2020

CDRIVER-3535 add mongoc_interrupt_t #610

Closed

kevinAlbs requested a review from alcaeus May 13, 2020 13:47

alcaeus reviewed May 19, 2020

View reviewed changes

address feedback

422fa35

kevinAlbs commented May 21, 2020

View reviewed changes

kevinAlbs mentioned this pull request May 25, 2020

CDRIVER-748 fix mock server for TSAN #620

Merged

bazile-clyde approved these changes May 29, 2020

View reviewed changes

kevinAlbs closed this May 29, 2020

eramongodb mentioned this pull request Jan 30, 2025

CDRIVER-4500: SDAM structured log and unified test support #1842

Merged

CDRIVER-3625 monitor with a thread-per-server #607

CDRIVER-3625 monitor with a thread-per-server #607

Uh oh!

Conversation

kevinAlbs commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bazile-clyde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinAlbs May 29, 2020 •

edited

Loading