Load balancer: SDAM and Server Selection #816

kevinAlbs · 2021-07-03T16:41:20Z

CDRIVER-4053 Load balancer: SDAM changes
- Add a LoadBalanced topology type and LoadBalancer server type.
- Manually set the topology description to contain one server of type LoadBalancer when opening.
- Prevent updates to the topology description after opening.
- Disable SDAM monitoring for a LoadBalanced topology.
- Emit correct SDAM monitoring events for a LoadBalanced topology.
- Bypass the cooldown period for the single-threaded topology scanner.
CDRIVER-4055 Load balancer: Server Selection
- Always return the one load balancer in server selection.

kevinAlbs · 2021-07-07T02:34:47Z

src/libmongoc/tests/json-test.c

@@ -317,9 +320,6 @@ process_sdam_test_hello_responses (bson_t *phase,
                                            generation);
         bson_mutex_unlock (&topology->mutex);
      }
-   } else {
-      test_error (
-         "test does not have 'responses' or 'applicationErrors' array");


The new discover_load_balancer SDAM test does not include the responses field. I think this is intentional since no hello responses need to be processed to transition to a LoadBalanced topology.

kevinAlbs · 2021-07-07T02:40:09Z

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

+#include "mongoc/mongoc-client-session-private.h"
+#include "test-conveniences.h"
+#include "test-libmongoc.h"
+#include "TestSuite.h"


These tests are not run on Evergreen. I have run this locally to verify, but will add to Evergreen as part of CDRIVER-4062.

What are these tests/where are they required in the spec?

These are not required by the spec. I would like to include them as a simple validation of the implementation up to this point.

Load balancer support is still incomplete. My process has been run a narrowly scoped failing test, make the minimal behavior change, then refactor. ("red" => "green" => "refactor").

The server selection JSON spec tests verify server selection works.

The SDAM JSON tests verify that SDAM is disabled.

/loadbalanced/connect/pooled and /loadbalanced/connect/single verify that sending a command uses the server description tied to the connection. Without the recent fix of CDRIVER-3653, those tests fail.

/loadbalanced/cooldown_is_bypassed/single verifies behavior specific to the C implementation. Connection creation in single-threaded goes through the same machinery as SDAM. Without an additional change to bypass the 5 second cooldown, a network error could wait 5 seconds before successfully reconnecting.

/loadbalanced/server_selection_establishes_connection/single verifies a requirement from wrapping drivers: successful server selection implies a connection was established to a server and is ready for commands.

Sounds good to me.

kevinAlbs · 2021-07-08T00:28:59Z

As I was working on handshake changes, I realize some crucial details are missing:

Topology updates are not being skipped on the hello response to application connections.
Single threaded connections are not getting created correctly since they are created as part of monitoring.

I will add integration tests before re-request review once these are resolved. Moving back to draft.

src/libmongoc/src/mongoc/mongoc-server-description-private.h

This reverts commit c18947e035b94e095d394dbd1764ee50a3fc9a3e.

…-threaded

kevinAlbs · 2021-07-19T00:18:06Z

src/libmongoc/src/mongoc/mongoc-server-description.c

@@ -807,6 +807,9 @@ mongoc_server_description_new_copy (
                                              &description->error);
   } else {
      mongoc_server_description_reset (copy);
+      /* preserve the original server description type, which is manually set


Note, this was not necessary for regular (non load balanced) server descriptions, which determine the type when processing the last last_hello_response.

But a load balanced server description does not have a hello response. It is manually created.

kevinAlbs · 2021-07-19T00:27:15Z

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

@@ -352,6 +352,10 @@ _mongoc_topology_description_server_is_candidate (
      default:
         return false;
      }
+
+   case MONGOC_TOPOLOGY_LOAD_BALANCED:


Note, there is currently no call path to this function. But I added a case to silence compiler warnings on an unhandled enum value.

Load balanced server selection starts in _mongoc_topology_select_server_id_loadbalanced => mongoc_topology_description_select => mongoc_topology_description_suitable_servers.

I alternatively considered making this a BSON_ASSERT (false) since this is untested. But it seemed low impact, and this is the behavior I'd expect if _mongoc_topology_description_server_is_candidate was called on a load balancer topology type.

A brief comment to that effect in the code might be nice, as this GitHub context won't easily be available to the reader.

kevinAlbs · 2021-07-19T00:33:45Z

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

+          * create connections. The cooldown period does not apply. A network
+          * error to a load balanced connection does not imply subsequent
+          * connection attempts will be to the same server and that a delay
+          * should occur. */


The cooldown period is described in the server monitoring specification.

Single threaded connections are created in the topology scanner so go through the machinery that does SDAM monitoring. But in load balanced mode, once a connection is created, it is never monitored. If the connection is closed (e.g. through a network error) the scanner should create a new connection as soon as one is requested.

kevinAlbs · 2021-07-19T00:37:56Z

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

@@ -86,6 +96,41 @@ _topology_has_description (mongoc_topology_description_t *topology,
                      BSON_ITER_HOLDS_INT32 (&iter));
         expected_generation = bson_iter_int32 (&iter);
         ASSERT_CMPINT32 (expected_generation, ==, sd->generation);
+      } else if (strcmp ("logicalSessionTimeoutMinutes",


These changes are to support the new discover_load_balancer.yml test. The test asserts many fields on a load balancer are unset by setting the expectation to null.

chardan

This looks good! I think I only see one thing that probably should be addressed (mongoc_server_description_new_copy() is unchecked), otherwise +1 from me-- but please
bear in mind that I'm not too familiar with this behavior or the codebase yet.

chardan · 2021-07-22T00:22:25Z

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

+       * an error would have occurred when constructing the topology. */
+      BSON_ASSERT (td->servers->items_len == 1);
+      sd = (mongoc_server_description_t *) mongoc_set_get_item (td->servers, 0);
+      prev_sd = mongoc_server_description_new_copy (sd);


Note that mongoc_server_description_new_copy() can return NULL on failure.

Good point! Added a BSON_ASSERT since this would be an OOM error.

chardan · 2021-07-22T00:36:08Z

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

+   if (!selected_server) {
+      _mongoc_server_selection_error (
+         "No suitable server found in load balanced deployment", NULL, error);
+      bson_mutex_unlock (&topology->mutex);


nit: It looks like what you've written here correctly releases the mutex, but you might also consider the goto-return idiom, which makes this somewhat easier to clean up in the face of new code being added.

I see, you might want to release the mutex as soon as possible, rather than holding it for the duration of the function. Ok. (Although in that case, would it be better as its own function? Tough call here IMO-- it's fine as-is.)

Yes, in this case I did not want to hold the mutex for the remainder of the function. I will leave as-is.

chardan · 2021-07-22T00:46:40Z

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

+   int topology_changed_events;
+   int topology_opening_events;
+   int topology_closed_events;
+} stats_t;


Even though this is in test code, "stats_t" seems generic enough that it has a high chance of conflicting with another symbol that the test might link to.

chardan · 2021-07-22T00:49:44Z

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

+{
+   mongoc_apm_callbacks_t *cbs;
+
+   cbs = mongoc_apm_callbacks_new ();


note: ok to ignore result, bson_malloc0() promises to call abort() on failure.

chardan · 2021-07-22T00:56:29Z

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

+                          MONGOC_ERROR_STREAM_NOT_ESTABLISHED,
+                          "Could not establish stream");
+
+   /* Failing to "scan" would normally cause the node to be in cooldown and fail


benjirewis

LGTM mod @chardan's comments. Thanks again for all the offline explanation.

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

benjirewis · 2021-07-22T14:53:33Z

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

+#include "mongoc/mongoc-client-session-private.h"
+#include "test-conveniences.h"
+#include "test-libmongoc.h"
+#include "TestSuite.h"


Sounds good to me.

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

kevinAlbs force-pushed the sdam.loadbalancer branch 2 times, most recently from b83116d to b346c4e Compare July 7, 2021 02:29

kevinAlbs commented Jul 7, 2021

View reviewed changes

kevinAlbs requested a review from jmikola July 7, 2021 02:46

kevinAlbs marked this pull request as ready for review July 7, 2021 02:46

kevinAlbs removed the request for review from jmikola July 8, 2021 00:27

kevinAlbs changed the title ~~Load balancer: SDAM, Server Selection, and Sessions~~ [WIP] Load balancer: SDAM, Server Selection, and Sessions Jul 8, 2021

alcaeus reviewed Jul 8, 2021

View reviewed changes

src/libmongoc/src/mongoc/mongoc-server-description-private.h Outdated Show resolved Hide resolved

kevinAlbs marked this pull request as draft July 14, 2021 21:01

kevinAlbs force-pushed the sdam.loadbalancer branch from 0fb7eef to 472242b Compare July 15, 2021 00:43

kevinAlbs changed the title ~~[WIP] Load balancer: SDAM, Server Selection, and Sessions~~ [WIP] Load balancer: SDAM and Server Selection Jul 15, 2021

kevinAlbs mentioned this pull request Jul 15, 2021

CDRIVER-4056 Send LB in handshake and reject non-LB connections #828

Merged

kevinAlbs added 15 commits July 18, 2021 19:46

add server selection and SDAM monitoring spec tests

2ed28e0

Disable SDAM on LoadBalanced, add events

0a3200e

CDRIVER-4055 Update server selection for load balancer

e23415f

workaround for CDRIVER-4057

962117a

shortcut server selection

118fb60

fix mutex unlock

a187f86

WIP: fixing connection logic

1db7161

Revert "WIP: fixing connection logic"

5dd8c50

This reverts commit c18947e035b94e095d394dbd1764ee50a3fc9a3e.

comment

e0998b3

rebase fixes

4d929fe

bypass server selection; ensure connections are established in single…

d0dde5e

…-threaded

add APM to tests

cac6bae

bypass topology description updates; bypass cooldown

4efc1a8

fix c89 and scan-build

79304fd

update comments; format

83f9d8f

kevinAlbs force-pushed the sdam.loadbalancer branch from d1c6507 to 83f9d8f Compare July 19, 2021 00:10

update log

80a7154

kevinAlbs commented Jul 19, 2021

View reviewed changes

kevinAlbs changed the title ~~[WIP] Load balancer: SDAM and Server Selection~~ Load balancer: SDAM and Server Selection Jul 19, 2021

kevinAlbs marked this pull request as ready for review July 19, 2021 13:36

kevinAlbs requested review from benjirewis and chardan July 19, 2021 13:50

add comment

7a566e1

chardan approved these changes Jul 22, 2021

View reviewed changes

benjirewis approved these changes Jul 22, 2021

View reviewed changes

benji / jesse comments

fc18c9c

kevinAlbs merged commit f9db623 into mongodb:master Jul 22, 2021

kevinAlbs deleted the sdam.loadbalancer branch July 22, 2021 21:13

kevinAlbs mentioned this pull request Jul 26, 2021

CDRIVER-4058 Load balancer: Error handling #831

Merged

kevinAlbs restored the sdam.loadbalancer branch February 3, 2022 14:28

Load balancer: SDAM and Server Selection #816

Load balancer: SDAM and Server Selection #816

Uh oh!

Conversation

kevinAlbs commented Jul 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Jul 8, 2021

Uh oh!

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!

chardan 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!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinAlbs commented Jul 3, 2021 •

edited

Loading