PYTHON-2676 Add load balancer tests in EVG #625

ShaneHarvey · 2021-05-19T20:06:06Z

This PR adds the load balancer test suite. Note that I have removed the "load_balancer/unified" spec tests that depend on connection pinning for cursors+transactions.

ShaneHarvey · 2021-05-21T23:40:37Z

Updated to fix some tests by implementing service_id property on various command events. Then I removed all the test files that depend on connection pinning which I would prefer to implement in a standalone PR. The lone remaining failure is:

 [2021/05/21 22:28:44.460] ----------------------------------------------------------------------
 [2021/05/21 22:28:44.460] Traceback (most recent call last):
 [2021/05/21 22:28:44.460]   File "/data/mci/efdc4de973353c508cc5913a6161c4c0/src/test/unified_format.py", line 1079, in test_case
 [2021/05/21 22:28:44.460]     self.run_scenario(spec)
 [2021/05/21 22:28:44.460]   File "/data/mci/efdc4de973353c508cc5913a6161c4c0/src/test/unified_format.py", line 1063, in run_scenario
 [2021/05/21 22:28:44.460]     self.run_operations(spec['operations'])
 [2021/05/21 22:28:44.460]   File "/data/mci/efdc4de973353c508cc5913a6161c4c0/src/test/unified_format.py", line 997, in run_operations
 [2021/05/21 22:28:44.460]     self.run_special_operation(op)
 [2021/05/21 22:28:44.460]   File "/data/mci/efdc4de973353c508cc5913a6161c4c0/src/test/unified_format.py", line 989, in run_special_operation
 [2021/05/21 22:28:44.460]     method(spec['arguments'])
 [2021/05/21 22:28:44.460]   File "/data/mci/efdc4de973353c508cc5913a6161c4c0/src/test/unified_format.py", line 924, in _testOperation_assertSessionUnpinned
 [2021/05/21 22:28:44.460]     self.assertIsNone(session._transaction.pinned_address)
 [2021/05/21 22:28:44.460] AssertionError: ('127.0.0.1', 8001) is not None

~ I suspect this is a bug in the spec test itself. I'm trying to determine how other drivers pass this test.~
Edit: the bug was that the unified test runner was incorrectly evaluating runOnRequirements. Fixed.

ShaneHarvey

Note the failures on latest-non-LB clusters will be fixed by #626

ShaneHarvey · 2021-05-22T00:20:38Z

test/unified_format.py

@@ -173,28 +204,28 @@ def __init__(self, test_class):
        self._entities = {}
        self._listeners = {}
        self._session_lsids = {}
-        self._test_class = test_class
+        self.test = test_class


I renamed this from _test_class to test to save horizontal space and avoid needing to break up as many lines.

ShaneHarvey · 2021-05-22T00:21:23Z

test/unified_format.py

@@ -142,28 +149,52 @@ def parse_bulk_write_error_result(error):
    return parse_bulk_write_result(write_result)


-class EventListenerUtil(CommandListener):
+class NonLazyCursor(object):


NonLazyCursor is used for the new createFindCursor operation.

I am not sure I follow - why is this needed?

createFindCursor mandates that the cursor is actually created on the server but not iterated: https://github.com/mongodb/specifications/blob/master/source/unified-test-format/unified-test-format.rst#createfindcursor

Add loadBalanced=true to default test client opts.

…ignoreResultAndError

…cking into pool Fixes: FAIL: test_sessions_are_reused_in_LB_mode (test_load_balancer.TestUnifiedTransactions) ---------------------------------------------------------------------- Traceback (most recent call last): File "/data/mci/941f91603ef9b23e469965893d64cc4e/src/test/unified_format.py", line 1071, in test_case self.run_scenario(spec) File "/data/mci/941f91603ef9b23e469965893d64cc4e/src/test/unified_format.py", line 1055, in run_scenario self.run_operations(spec['operations']) File "/data/mci/941f91603ef9b23e469965893d64cc4e/src/test/unified_format.py", line 989, in run_operations self.run_special_operation(op) File "/data/mci/941f91603ef9b23e469965893d64cc4e/src/test/unified_format.py", line 981, in run_special_operation method(spec['arguments']) File "/data/mci/941f91603ef9b23e469965893d64cc4e/src/test/unified_format.py", line 934, in _testOperation_assertSameLsidOnLastTwoCommands self.assertEqual(*self.__get_last_two_command_lsids(listener)) AssertionError: {'id': Binary(b'\x06\xa6)\xfc\xe5QK/\x80"\xab\xc9]\xb2Tr', 4)} != {'id': Binary(b'\x9c\x86\x19/s\x96O\x85\xa6\x93k\x92d\xff{\x8b', 4)} - {'id': Binary(b'\x06\xa6)\xfc\xe5QK/\x80"\xab\xc9]\xb2Tr', 4)} + {'id': Binary(b'\x9c\x86\x19/s\x96O\x85\xa6\x93k\x92d\xff{\x8b', 4)}

…fied test assertion

ShaneHarvey · 2021-05-24T21:08:49Z

Rebased to make the tests green.

prashantmital · 2021-05-25T09:16:55Z

.evergreen/run-tests.sh

@@ -51,6 +51,11 @@ fi
 if [ "$SSL" != "nossl" ]; then
    export CLIENT_PEM="$DRIVERS_TOOLS/.evergreen/x509gen/client.pem"
    export CA_PEM="$DRIVERS_TOOLS/.evergreen/x509gen/ca.pem"
+
+    if [ -n "$TEST_LOADBALANCER" ]; then
+        export SINGLE_MONGOS_LB_URI="${SINGLE_MONGOS_LB_URI}&tls=true"


Why can't the tls=true be part of the evergreen variable containing the URI instead of having to patch it up here?

I cribbed this from the Java driver. I think the underlying issue is that MO does not yet add SSL/TLS parameters to the uri it reports: 10gen/mongo-orchestration#287

prashantmital · 2021-05-25T09:17:57Z

.evergreen/run-tests.sh

-    $PYTHON $COVERAGE_ARGS setup.py $C_EXTENSIONS test $TEST_ARGS $OUTPUT
+
+    if [ -n "$TEST_LOADBALANCER" ]; then
+        $PYTHON -m unittest discover -s test/load_balancer -v


Does invoking tests like this still generate the XML output?

Good point. Updated to use the unittest-xml-reporting command line tool.

prashantmital · 2021-05-25T09:19:34Z

pymongo/mongo_client.py


    def contribute_socket(self, sock_info):
        """Provide socket information to the error handler."""
        self.max_wire_version = sock_info.max_wire_version
        self.sock_generation = sock_info.generation
+        self.service_id = sock_info.service_id


What is service_id when there is no LB?

It's always None when not in LB mode. When in LB mode there is a single connection pool and service_id is used to identify connections to the same host (and differentiate connections to different hosts). Take a peak at #628 and https://github.com/mongodb/specifications/blob/master/source/load-balancers/load-balancers.rst#error-handling for more info.

prashantmital · 2021-05-25T09:21:37Z

test/unified_format.py

@@ -142,28 +149,52 @@ def parse_bulk_write_error_result(error):
    return parse_bulk_write_result(write_result)


-class EventListenerUtil(CommandListener):
+class NonLazyCursor(object):


I am not sure I follow - why is this needed?

prashantmital · 2021-05-25T20:57:24Z

.evergreen/config.yml

+          DRIVERS_TOOLS=${DRIVERS_TOOLS} MONGODB_URI=${MONGODB_URI} bash ${DRIVERS_TOOLS}/.evergreen/run-load-balancer.sh start
+    - command: expansions.update
+      params:
+        file: lb-expansion.yml


How/where is this being used?

run-load-balancer.sh creates this file, see: https://github.com/mongodb/specifications/blob/master/source/load-balancers/tests/README.rst

prashantmital · 2021-05-25T20:58:55Z

.evergreen/config.yml

+    platform: ubuntu-18.04
+    mongodb-version: ["latest"]
+    auth-ssl: "*"
+    python-version: ["3.6", "3.9"]


Whats the reason for only testing these Pythons? What about pypy? We needn't change it, but a comment with the rationale will be useful and maybe an accompanying ticket if the testing needs to be expanded at a later point.

For now it's just for simplicity. I opened: https://jira.mongodb.org/browse/PYTHON-2731

I might just add all python versions while doing that ticket.

prashantmital · 2021-05-25T20:59:41Z

.evergreen/run-tests.sh

@@ -51,6 +51,11 @@ fi
 if [ "$SSL" != "nossl" ]; then
    export CLIENT_PEM="$DRIVERS_TOOLS/.evergreen/x509gen/client.pem"
    export CA_PEM="$DRIVERS_TOOLS/.evergreen/x509gen/ca.pem"
+
+    if [ -n "$TEST_LOADBALANCER" ]; then
+        export SINGLE_MONGOS_LB_URI="${SINGLE_MONGOS_LB_URI}&tls=true"


prashantmital · 2021-05-25T21:00:05Z

.evergreen/run-tests.sh

-    $PYTHON $COVERAGE_ARGS setup.py $C_EXTENSIONS test $TEST_ARGS $OUTPUT
+
+    if [ -n "$TEST_LOADBALANCER" ]; then
+        $PYTHON -m xmlrunner discover -s test/load_balancer -v --locals -o $XUNIT_DIR


What does --locals do?

It prints each function's local variables in stack traces: https://docs.python.org/3/library/unittest.html#cmdoption-unittest-locals

So this opaque trace:

FAIL: test_a_connection_can_be_shared_by_a_transaction_and_a_cursor (test_load_balancer.TestUnifiedTransactions) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 1087, in test_case self.run_scenario(spec) File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 1074, in run_scenario self.check_events(spec.get('expectEvents', [])) File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 1026, in check_events self.match_evaluator.match_event( File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 581, in match_event self.test.assertIsInstance(actual, ConnectionCheckedInEvent) AssertionError: ConnectionReadyEvent(('127.0.0.1', 8001), 2) is not an instance of <class 'pymongo.monitoring.ConnectionCheckedInEvent'>

Becomes much more diagnosable:

FAIL: test_a_connection_can_be_shared_by_a_transaction_and_a_cursor (test_load_balancer.TestUnifiedTransactions) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 1087, in test_case self.run_scenario(spec) self = <test_load_balancer.TestUnifiedTransactions testMethod=test_a_connection_can_be_shared_by_a_transaction_and_a_cursor> spec = {'description': 'a connection can be shared by a transaction and a cursor', 'operations': [{'name': 'startTransaction', 'object': 'session0'}, {'name': 'insertOne', 'object': 'collection0', 'arguments': {'document': {'x': 1}, 'session': 'session0'}}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 1}}, {'name': 'createFindCursor', 'object': 'collection0', 'arguments': {'filter': {}, 'batchSize': 2, 'session': 'session0'}, 'saveResultAsEntity': 'cursor0'}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 1}}, {'name': 'close', 'object': 'cursor0'}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 1}}, {'name': 'abortTransaction', 'object': 'session0'}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 0}}], 'expectEvents': [{'client': 'client0', 'events': [{'commandStartedEvent': {'commandName': 'insert'}}, {'commandStartedEvent': {'commandName': 'find'}}, {'commandStartedEvent': {'commandName': 'killCursors'}}, {'commandStartedEvent': {'commandName': 'abortTransaction'}}]}, {'client': 'client0', 'eventType': 'cmap', 'events': [{'connectionReadyEvent': {}}, {'connectionCheckedOutEvent': {}}, {'connectionCheckedInEvent': {}}]}]} File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 1074, in run_scenario self.check_events(spec.get('expectEvents', [])) run_on_spec = [] self = <test_load_balancer.TestUnifiedTransactions testMethod=test_a_connection_can_be_shared_by_a_transaction_and_a_cursor> skip_reason = None spec = {'description': 'a connection can be shared by a transaction and a cursor', 'operations': [{'name': 'startTransaction', 'object': 'session0'}, {'name': 'insertOne', 'object': 'collection0', 'arguments': {'document': {'x': 1}, 'session': 'session0'}}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 1}}, {'name': 'createFindCursor', 'object': 'collection0', 'arguments': {'filter': {}, 'batchSize': 2, 'session': 'session0'}, 'saveResultAsEntity': 'cursor0'}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 1}}, {'name': 'close', 'object': 'cursor0'}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 1}}, {'name': 'abortTransaction', 'object': 'session0'}, {'name': 'assertNumberConnectionsCheckedOut', 'object': 'testRunner', 'arguments': {'client': 'client0', 'connections': 0}}], 'expectEvents': [{'client': 'client0', 'events': [{'commandStartedEvent': {'commandName': 'insert'}}, {'commandStartedEvent': {'commandName': 'find'}}, {'commandStartedEvent': {'commandName': 'killCursors'}}, {'commandStartedEvent': {'commandName': 'abortTransaction'}}]}, {'client': 'client0', 'eventType': 'cmap', 'events': [{'connectionReadyEvent': {}}, {'connectionCheckedOutEvent': {}}, {'connectionCheckedInEvent': {}}]}]} File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 1026, in check_events self.match_evaluator.match_event( actual_events = [ConnectionReadyEvent(('127.0.0.1', 8001), 1), ConnectionCheckedOutEvent(('127.0.0.1', 8001), 1), ConnectionReadyEvent(('127.0.0.1', 8001), 2), ConnectionCheckedOutEvent(('127.0.0.1', 8001), 2), ConnectionCheckedInEvent(('127.0.0.1', 8001), 2), ConnectionCheckedInEvent(('127.0.0.1', 8001), 1)] client_name = 'client0' event_spec = {'client': 'client0', 'eventType': 'cmap', 'events': [{'connectionReadyEvent': {}}, {'connectionCheckedOutEvent': {}}, {'connectionCheckedInEvent': {}}]} event_type = 'cmap' events = [{'connectionReadyEvent': {}}, {'connectionCheckedOutEvent': {}}, {'connectionCheckedInEvent': {}}] expected_event = {'connectionCheckedInEvent': {}} idx = 2 listener = <test.unified_format.EventListenerUtil object at 0x7fc94603f3a0> self = <test_load_balancer.TestUnifiedTransactions testMethod=test_a_connection_can_be_shared_by_a_transaction_and_a_cursor> spec = [{'client': 'client0', 'events': [{'commandStartedEvent': {'commandName': 'insert'}}, {'commandStartedEvent': {'commandName': 'find'}}, {'commandStartedEvent': {'commandName': 'killCursors'}}, {'commandStartedEvent': {'commandName': 'abortTransaction'}}]}, {'client': 'client0', 'eventType': 'cmap', 'events': [{'connectionReadyEvent': {}}, {'connectionCheckedOutEvent': {}}, {'connectionCheckedInEvent': {}}]}] File "/Users/shane/git/mongo-python-driver/test/unified_format.py", line 581, in match_event self.test.assertIsInstance(actual, ConnectionCheckedInEvent) actual = ConnectionReadyEvent(('127.0.0.1', 8001), 2) event_type = 'cmap' expectation = {'connectionCheckedInEvent': {}} name = 'connectionCheckedInEvent' self = <test.unified_format.MatchEvaluatorUtil object at 0x7fc945968cd0> spec = {} AssertionError: ConnectionReadyEvent(('127.0.0.1', 8001), 2) is not an instance of <class 'pymongo.monitoring.ConnectionCheckedInEvent'>

Add load balancer spec tests Ensure LB supports retryable reads/writes Add assertNumberConnectionsCheckedOut, createFindCursor, ignoreResultAndError Add PoolClearedEvent.service_id and fix isClientError unified test assertion (cherry picked from commit 93ac5e0)

This comment has been minimized.

Sign in to view

ShaneHarvey marked this pull request as ready for review May 22, 2021 00:07

ShaneHarvey requested a review from prashantmital May 22, 2021 00:08

ShaneHarvey changed the title ~~PYTHON-2672 Add load balancer tests in EVG~~ PYTHON-2676 Add load balancer tests in EVG May 22, 2021

ShaneHarvey commented May 22, 2021

View reviewed changes

ShaneHarvey added 12 commits May 24, 2021 14:07

PYTHON-2672 Add load balancer tests in EVG

241f400

Add loadBalanced=true to default test client opts.

PYTHON-2676 Add load balancer spec tests

492421c

PYTHON-2676 Add assertNumberConnectionsCheckedOut, createFindCursor, …

e3a8c88

…ignoreResultAndError

PYTHON-2676 Pin to load balancer server address

f557b6c

PYTHON-2676 Ensure LB supports retryable reads/writes

5e7aaef

PYTHON-2676 Support useMultipleMongoses for LB

a51b4e6

PYTHON-2676 Support CMAP events

d0b8f4a

PYTHON-2676 Add PoolClearedEvent.service_id and fix isClientError uni…

a35b2aa

…fied test assertion

PYTHON-2676 Remove tests that rely on connection pinning

fe91291

Fix pool reset in tests and PoolClearedEvent service_id arg

16c251f

Fix runOnRequirements for load-balancer

dc940cc

ShaneHarvey force-pushed the PYTHON-2676 branch from 19cd63d to dc940cc Compare May 24, 2021 21:07

prashantmital reviewed May 25, 2021

View reviewed changes

ShaneHarvey added 2 commits May 25, 2021 09:28

Generate xunit output, show local variables in tracebacks

e08c68f

xml output dir must be absolute path

dc3eb57

ShaneHarvey requested a review from prashantmital May 25, 2021 20:53

prashantmital reviewed May 25, 2021

View reviewed changes

prashantmital approved these changes May 25, 2021

View reviewed changes

ShaneHarvey merged commit 93ac5e0 into mongodb:master May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PYTHON-2676 Add load balancer tests in EVG #625

PYTHON-2676 Add load balancer tests in EVG #625

ShaneHarvey commented May 19, 2021 •

edited

Loading

This comment has been minimized.

ShaneHarvey commented May 21, 2021 •

edited

Loading

ShaneHarvey left a comment

ShaneHarvey May 22, 2021

ShaneHarvey May 22, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021

ShaneHarvey commented May 24, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021

prashantmital May 25, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021

ShaneHarvey May 25, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021

prashantmital May 25, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021 •

edited

Loading

prashantmital May 25, 2021

prashantmital May 25, 2021

ShaneHarvey May 25, 2021

PYTHON-2676 Add load balancer tests in EVG #625

PYTHON-2676 Add load balancer tests in EVG #625

Conversation

ShaneHarvey commented May 19, 2021 • edited Loading

This comment has been minimized.

ShaneHarvey commented May 21, 2021 • edited Loading

ShaneHarvey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaneHarvey commented May 19, 2021 •

edited

Loading

ShaneHarvey commented May 21, 2021 •

edited

Loading

ShaneHarvey May 25, 2021 •

edited

Loading