Re: [Tarantool-patches] [PATCH 1/1] test: fix flaky unit/swim test

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi Vlad, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/218165816

On Wed, Nov 18, 2020 at 10:24:41PM +0100, Vladislav Shpilevoy wrote:
> swim_test_indirect_ping() failed with random seed 1605651752.
> 
> The test created a cluster with 3 swim nodes, and broke network
> connection between node-1 and node-2. Then it run the cluster for
> 10 seconds, and ensured, that both node-1 and node-2 are
> eventually alive despite they are suspected sometimes.
> 
>     node1 <-> node3 <-> node2
> 
> 'Alive' means that a node is considered alive on all the other
> nodes.
> 
> The test spun for 10 seconds giving the nodes a chance to become
> suspected. Then it checked that node-1 is either still alive, or
> it is suspected, but will be restored in at most 3 seconds. The
> same was checked for node-2. They were supposed to interact via
> node-3.
> 
> 3 seconds was used assuming that the worst what could happen is
> that it is suspected from the beginning of this three-second
> interval on node-3, because it was suspected by node-2 and
> disseminated to node-3.
> 
> Then node-3 might need 1 second to finish its current
> dissemination round by sending a ping to node-2, 1 second to start
> new round randomly again from node-2, and only then send a ping to
> node-1. So 3 seconds total.
> 
> But it could also happen, that in the beginning of the
> three-second interval node-1 is already suspected on node-2. On
> the next step node-2 shares the suspicion with node-3. And then
> the scenario above happens. So the test case needed at least 4
> seconds.
> 
> And actually it could happen infinitely, because while the test
> waits for 3 seconds of gossip refutation about node-1 on node-3,
> node-2 can suspect it again. And so on.
> 
> Also the test would pass even without indirect pings. Because
> node-3 has access to node-1 and node-2. So even if, say, node-1
> suspects node-2, then it will tell node-3 about it. Node-3 will
> ping node-2, get ack, and will refute the gossip. The refutation
> will be then sent to node-1 back. It means indirect pings don't
> matter here.
> 
> The patch makes a new test, which won't pass without indirect
> pings. It uses the existing error injection ERRINJ_SWIM_FD_ONLY,
> which allows to turn off all the SWIM components except failure
> detection. So only pings and acks are being sent.
> 
> Then without proper indirect pings node-1 and node-2 would suspect
> each other and declare dead eventually. The new test checks it
> does not happen.
> 
> Closes #5399
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5399-swim-flaky
> Issue: https://github.com/tarantool/tarantool/issues/5399
> 
>  test/unit/suite.ini          |  8 -------
>  test/unit/swim.c             | 26 +----------------------
>  test/unit/swim.result        | 20 ++++++------------
>  test/unit/swim_errinj.c      | 41 +++++++++++++++++++++++++++++++++++-
>  test/unit/swim_errinj.result |  8 ++++++-
>  5 files changed, 55 insertions(+), 48 deletions(-)
> 
> diff --git a/test/unit/suite.ini b/test/unit/suite.ini
> index fe871b287..d16ba413b 100644
> --- a/test/unit/suite.ini
> +++ b/test/unit/suite.ini
> @@ -4,11 +4,3 @@ description = unit tests
>  disabled = snap_quorum_delay.test
>  release_disabled = fiber_stack.test swim_errinj.test
>  is_parallel = True
> -fragile = {
> -    "retries": 10,
> -    "tests": {
> -        "swim.test": {
> -            "issues": [ "gh-5399" ]
> -        }
> -    }
> -  }
> diff --git a/test/unit/swim.c b/test/unit/swim.c
> index bb12baf8d..4e9dce8ce 100644
> --- a/test/unit/swim.c
> +++ b/test/unit/swim.c
> @@ -735,29 +735,6 @@ swim_test_payload_basic(void)
>  	swim_finish_test();
>  }
>  
> -static void
> -swim_test_indirect_ping(void)
> -{
> -	swim_start_test(2);
> -	uint16_t cluster_size = 3;
> -	struct swim_cluster *cluster = swim_cluster_new(cluster_size);
> -	swim_cluster_set_ack_timeout(cluster, 1);
> -	for (int i = 0; i < cluster_size; ++i) {
> -		for (int j = i + 1; j < cluster_size; ++j)
> -			swim_cluster_interconnect(cluster, i, j);
> -	}
> -	swim_cluster_set_drop_channel(cluster, 0, 1, true);
> -	swim_cluster_set_drop_channel(cluster, 1, 0, true);
> -	swim_run_for(10);
> -	is(swim_cluster_wait_status_everywhere(cluster, 0, MEMBER_ALIVE, 3),
> -	   0, "S1 is still alive everywhere");
> -	is(swim_cluster_wait_status_everywhere(cluster, 1, MEMBER_ALIVE, 3),
> -	   0, "as well as S2 - they communicated via S3");
> -
> -	swim_cluster_delete(cluster);
> -	swim_finish_test();
> -}
> -
>  static void
>  swim_test_encryption(void)
>  {
> @@ -1097,7 +1074,7 @@ swim_test_suspect_new_members(void)
>  static int
>  main_f(va_list ap)
>  {
> -	swim_start_test(23);
> +	swim_start_test(22);
>  
>  	(void) ap;
>  	swim_test_ev_init();
> @@ -1119,7 +1096,6 @@ main_f(va_list ap)
>  	swim_test_uri_update();
>  	swim_test_broadcast();
>  	swim_test_payload_basic();
> -	swim_test_indirect_ping();
>  	swim_test_encryption();
>  	swim_test_slow_net();
>  	swim_test_triggers();
> diff --git a/test/unit/swim.result b/test/unit/swim.result
> index 07ea6a1a0..d4b1dba49 100644
> --- a/test/unit/swim.result
> +++ b/test/unit/swim.result
> @@ -1,5 +1,5 @@
>  	*** main_f ***
> -1..23
> +1..22
>  	*** swim_test_one_link ***
>      1..6
>      ok 1 - no rounds - no fullmesh
> @@ -169,23 +169,17 @@ ok 15 - subtests
>      ok 11 - third payload is disseminated via anti-entropy
>  ok 16 - subtests
>  	*** swim_test_payload_basic: done ***
> -	*** swim_test_indirect_ping ***
> -    1..2
> -    ok 1 - S1 is still alive everywhere
> -    ok 2 - as well as S2 - they communicated via S3
> -ok 17 - subtests
> -	*** swim_test_indirect_ping: done ***
>  	*** swim_test_encryption ***
>      1..3
>      ok 1 - cluster works with encryption
>      ok 2 - different encryption keys - can't interact
>      ok 3 - cluster works after encryption has been disabled
> -ok 18 - subtests
> +ok 17 - subtests
>  	*** swim_test_encryption: done ***
>  	*** swim_test_slow_net ***
>      1..0
>      # slow network leads to idle round steps, they should not produce a new message
> -ok 19 - subtests
> +ok 18 - subtests
>  	*** swim_test_slow_net: done ***
>  	*** swim_test_triggers ***
>      1..20
> @@ -210,25 +204,25 @@ ok 19 - subtests
>      # now all the triggers are done and deleted
>      ok 19 - local URI update warns about version update
>      ok 20 - version is a part of incarnation, so the latter is updated too
> -ok 20 - subtests
> +ok 19 - subtests
>  	*** swim_test_triggers: done ***
>  	*** swim_test_generation ***
>      1..3
>      ok 1 - S1 disseminated its payload to S2
>      ok 2 - S1 restarted and set another payload. Without generation it could lead to never disseminated new payload.
>      ok 3 - S2 sees new generation of S1
> -ok 21 - subtests
> +ok 20 - subtests
>  	*** swim_test_generation: done ***
>  	*** swim_test_dissemination_speed ***
>      1..2
>      ok 1 - dissemination work in log time even at the very start of a cluster
>      ok 2 - dissemination can withstand an event storm
> -ok 22 - subtests
> +ok 21 - subtests
>  	*** swim_test_dissemination_speed: done ***
>  	*** swim_test_suspect_new_members ***
>      1..2
>      ok 1 - S2 dropped S1 as dead
>      ok 2 - S3 didn't add S1 from S2's messages, because S1 didn't answer on a ping
> -ok 23 - subtests
> +ok 22 - subtests
>  	*** swim_test_suspect_new_members: done ***
>  	*** main_f: done ***
> diff --git a/test/unit/swim_errinj.c b/test/unit/swim_errinj.c
> index 2e89f1821..0f8718a33 100644
> --- a/test/unit/swim_errinj.c
> +++ b/test/unit/swim_errinj.c
> @@ -171,17 +171,56 @@ swim_test_payload_refutation(void)
>  	swim_finish_test();
>  }
>  
> +static void
> +swim_test_indirect_ping(void)
> +{
> +	swim_start_test(2);
> +	uint16_t cluster_size = 3;
> +	struct swim_cluster *cluster = swim_cluster_new(cluster_size);
> +	swim_cluster_set_ack_timeout(cluster, 0.5);
> +	for (int i = 0; i < cluster_size; ++i) {
> +		for (int j = i + 1; j < cluster_size; ++j)
> +			swim_cluster_interconnect(cluster, i, j);
> +	}
> +	swim_cluster_set_drop_channel(cluster, 0, 1, true);
> +	swim_cluster_set_drop_channel(cluster, 1, 0, true);
> +	/*
> +	 * There are alive channels S1 <-> S3 and S2 <-> S3. Things are not
> +	 * interesting when S3 can send dissemination, because even if S1 and S2
> +	 * suspect each other, they will share it with S3, and S3 will refute it
> +	 * via dissemination, because has access to both of them.
> +	 * When only failure detection works, a suspicion can only be refuted by
> +	 * pings and acks. In this test pings-acks between S1 and S2 will need
> +	 * to be indirect.
> +	 */
> +	struct errinj *errinj = &errinjs[ERRINJ_SWIM_FD_ONLY];
> +	errinj->bparam = true;
> +	/*
> +	 * No nodes are ever suspected. Because when a direct ping fails, the
> +	 * nodes simply will use indirect pings and they will work.
> +	 */
> +	isnt(swim_cluster_wait_status_anywhere(cluster, 0,
> +					       MEMBER_SUSPECTED, 10),
> +	     0, "S1 is never suspected");
> +	isnt(swim_cluster_wait_status_anywhere(cluster, 1,
> +					       MEMBER_SUSPECTED, 10),
> +	     0, "S2 is never suspected");
> +	errinj->bparam = false;
> +	swim_cluster_delete(cluster);
> +	swim_finish_test();
> +}
>  
>  static int
>  main_f(va_list ap)
>  {
> -	swim_start_test(1);
> +	swim_start_test(2);
>  
>  	(void) ap;
>  	swim_test_ev_init();
>  	swim_test_transport_init();
>  
>  	swim_test_payload_refutation();
> +	swim_test_indirect_ping();
>  
>  	swim_test_transport_free();
>  	swim_test_ev_free();
> diff --git a/test/unit/swim_errinj.result b/test/unit/swim_errinj.result
> index c60389d7f..03e357e13 100644
> --- a/test/unit/swim_errinj.result
> +++ b/test/unit/swim_errinj.result
> @@ -1,5 +1,5 @@
>  	*** main_f ***
> -1..1
> +1..2
>  	*** swim_test_payload_refutation ***
>      1..11
>      ok 1 - S2 sees new version of S1
> @@ -15,4 +15,10 @@
>      ok 11 - S3 learns S1's payload from S2
>  ok 1 - subtests
>  	*** swim_test_payload_refutation: done ***
> +	*** swim_test_indirect_ping ***
> +    1..2
> +    ok 1 - S1 is never suspected
> +    ok 2 - S2 is never suspected
> +ok 2 - subtests
> +	*** swim_test_indirect_ping: done ***
>  	*** main_f: done ***
> -- 
> 2.24.3 (Apple Git-128)
>