Improved discovery backend #24

arcusfelis · 2023-08-23T21:11:45Z

Adresses MIM-2018.

Make cets_discovery module more async and better tested.
This should avoid long locking time at startup in some cases (for example, there are a lot of dead nodes returned by the discovery backend).

Changes:

Make discovery gen_server non-blocking.
Adding wait_for_ready functionality - could be useful to avoid not-fully ready servers to be exposed to the users (i.e. to avoid accepting incoming connection on a new node, before it copied the session table, for example)
Smarter timeouts for retries. We use a different timeout after an error. We also don't call get_nodes too often once started.
Specs.
More tests with better coverage.
Fixes small race conditions in insert_new tests (rare, but they were found by rerunning tests).
Use peer module to do RPCs.
Some netsplit tests.

You can find MIM branch that uses this code here: esl/MongooseIM#4111 (for testing)

Command to test helm (ensure you use k8s from Docker Desktop and having helm installed):

# Setup DB using MIM test scripts:
DB="pgsql" ./tools/setup-db.sh
# Ensure MIM stopped...
killall beam.smp
# Get helm scripts
git clone https://github.com/esl/MongooseHelm.git
cd MongooseHelm
# Ensure old hem cluster is dead
helm uninstall mim-test
# Test with these changes
helm install mim-test MongooseIM --set replicaCount=3 --set image.tag=PR-4111 --set persistentDatabase=rdbms --set rdbms.username=ejabberd --set rdbms.password=mongooseim_secret --set rdbms.database=ejabberd --set volatileDatabase=cets
# Check that all is fine in logs:
kubectl get pod
kubectl logs mongooseim-0
kubectl exec -it mongooseim-0 -- mongooseimctl cets systemInfo

Add a test

codecov · 2023-08-23T21:13:55Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +4.86% 🎉

Comparison is base (3144dc0) 91.01% compared to head (079b0e2) 95.88%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   91.01%   95.88%   +4.86%     
==========================================
  Files           8        8              
  Lines         434      510      +76     
==========================================
+ Hits          395      489      +94     
+ Misses         39       21      -18

Files Changed	Coverage Δ
src/cets.erl	`100.00% <ø> (+0.57%)`	⬆️
src/cets_discovery.erl	`91.85% <100.00%> (+34.70%)`	⬆️
src/cets_join.erl	`98.93% <100.00%> (+0.99%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Fixed dialyzer Fixed a race condition in test_disco_add_two_tables testcase

peer:rpc is pretty cool: it allows to apply code on nodes without touching erlang distribution protocol Which means we can actually setup discovery tests

Add insert_returns_when_netsplit/inserts_after_netsplit_reconnects testcases Add cets_seq group for netsplits

NelsonVides

Just a few very small comments

NelsonVides · 2023-08-30T13:12:45Z

src/cets_join.erl

+                local_servers => LocSet,
+                remote_servers => RemSet,


Won't these be more unreadable in the logs than a list? I'd say it depends on whether the list had duplicates. If ordsets are simply ordering the list into a data structure that is faster to read or calculate intersections then I'd prefer we print the list, it's just more readable 🤔

oh yea, I did the change for logs to make logs easier to read.
get_pids() does return [Server | ordeset()]. So, I can just force to use ordsets everywhere, no need to allow non-sorted lists.

NelsonVides · 2023-08-31T08:21:03Z

src/cets_discovery.erl

+    AvailableNodes = nodes(),
+    spawn_link(fun() ->
+        %% We only care about connected nodes here
+        %% We do not wanna try to connect here - we do it in ping_not_connected_nodes/1


Suggested change

%% We do not wanna try to connect here - we do it in ping_not_connected_nodes/1

%% We do not want to try to connect here - we do it in ping_not_connected_nodes/1

NelsonVides · 2023-08-31T08:26:18Z

src/cets_discovery.erl

+%% Returns timeout in milliseconds to retry calling the get_nodes function.
+%% get_nodes is called after add_table without waiting.
+%% It is also would be retried without waiting if should_retry_get_nodes set to true.
+-spec retry_type_to_timeout(retry_type()) -> 5000 | 1000 | 180000.
+retry_type_to_timeout(initial) -> 5000;
+retry_type_to_timeout(after_error) -> 1000;
+retry_type_to_timeout(regular) -> 180000.


I don't know from the top of my mind how many minutes 180000ms is, so maybe at a comment, 180000ms = 5min, or, call timer:minutes(5), to make it more readable (comment and direct value might be "more performant though" xD)

NelsonVides

LGTM 🔥

arcusfelis added 3 commits August 23, 2023 16:55

Make cets_discovery:add_table/2 async

d6d9c96

Add a test

Get nodes and do join in parallel in cets_discovery

83b4043

Add wait_for_ready

47fcdef

arcusfelis added 6 commits August 24, 2023 19:15

Add a comment

5b96e81

Add test_disco_add_table_twice testcase

024fb07

Add test_disco_handles_bad_node testcase

7c90507

Add test to ensure joins are retried

c1f7f46

Add different retry intervals

319e744

Add disco_uses_regular_retry_interval_in_the_regular_phase testcase

dd79960

arcusfelis force-pushed the better-disco branch from 47a5f01 to dd79960 Compare August 28, 2023 20:50

arcusfelis added 2 commits August 29, 2023 09:39

Add disco_handles_node_up_and_down

9fd4237

Fixed dialyzer Fixed a race condition in test_disco_add_two_tables testcase

Use peer:rpc

cbc87fd

peer:rpc is pretty cool: it allows to apply code on nodes without touching erlang distribution protocol Which means we can actually setup discovery tests

arcusfelis force-pushed the better-disco branch from 658f1a5 to acef28e Compare August 29, 2023 22:04

arcusfelis added 3 commits August 30, 2023 00:07

Fix race conditions in tests

def2015

Add insert_returns_when_netsplit/inserts_after_netsplit_reconnects testcases Add cets_seq group for netsplits

Add disco_connects_to_unconnected_node testcase

46d6e18

Add specs to new functions in cets_discovery

e0e9570

arcusfelis force-pushed the better-disco branch from acef28e to e0e9570 Compare August 29, 2023 22:10

arcusfelis marked this pull request as ready for review August 29, 2023 22:15

arcusfelis added 2 commits August 30, 2023 00:42

Add test_disco_file_appears testcase

1cf10f1

Add run_tracked_failed testcase

93f37bc

arcusfelis requested review from chrzaszcz and NelsonVides August 30, 2023 07:19

arcusfelis mentioned this pull request Aug 30, 2023

[WIP] Use better-disco branch of CETS esl/MongooseIM#4111

Closed

Use CT_PEER macro

9c955a8

NelsonVides reviewed Aug 31, 2023

View reviewed changes

arcusfelis added 4 commits August 31, 2023 13:04

Use ordsets for list of servers always

56ec0aa

Use timer module to return timeout values

2823629

Replace wanna with want to

690be9d

Add info about netsplit cases block

12b7b70

arcusfelis force-pushed the better-disco branch from 1d99195 to 12b7b70 Compare August 31, 2023 11:30

Add more specs

079b0e2

arcusfelis requested a review from NelsonVides August 31, 2023 11:36

NelsonVides approved these changes Aug 31, 2023

View reviewed changes

NelsonVides merged commit 21f0479 into main Aug 31, 2023
6 checks passed

NelsonVides deleted the better-disco branch August 31, 2023 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved discovery backend #24

Improved discovery backend #24

arcusfelis commented Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading

NelsonVides left a comment

NelsonVides Aug 30, 2023

arcusfelis Aug 31, 2023

NelsonVides Aug 31, 2023

NelsonVides Aug 31, 2023

NelsonVides left a comment

	%% We do not wanna try to connect here - we do it in ping_not_connected_nodes/1
	%% We do not want to try to connect here - we do it in ping_not_connected_nodes/1

Improved discovery backend #24

Improved discovery backend #24

Conversation

arcusfelis commented Aug 23, 2023 • edited Loading

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

NelsonVides left a comment

Choose a reason for hiding this comment

NelsonVides Aug 30, 2023

Choose a reason for hiding this comment

arcusfelis Aug 31, 2023

Choose a reason for hiding this comment

NelsonVides Aug 31, 2023

Choose a reason for hiding this comment

NelsonVides Aug 31, 2023

Choose a reason for hiding this comment

NelsonVides left a comment

Choose a reason for hiding this comment

arcusfelis commented Aug 23, 2023 •

edited

Loading

codecov bot commented Aug 23, 2023 •

edited

Loading