Adjustment of netsocket tests to the lossy mesh network limitation. #11654

tymoteuszblochmobica · 2019-10-08T15:15:01Z

Description

Netsocket tests are modified according to loosy mesh network environment.
Some tests are skipped.
The failures need to be investigated by the infra team.
Local network setup, passes all tests

Pull request type

[x ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@mikter
@SeppoTakalo
@michalpasztamobica

Release Notes

ciarmcom · 2019-10-08T17:00:32Z

@tymoteuszblochmobica, thank you for your changes.
@SeppoTakalo @mikter @michalpasztamobica @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

SeppoTakalo · 2019-10-09T06:08:33Z

TESTS/netsocket/dns/main.cpp

+    printf("MBED: IP address is '%s'\n", address ? address : "null");
+#if MBED_CONF_TARGET_NETWORK_DEFAULT_INTERFACE_TYPE == MESH
+    uint8_t addr_hex[16];
+    stoip6(address, strlen(address), addr_hex);
+    if ((addr_hex[0] & 0xe0) != 0x20) {
+        printf("Adress is not globlal  Please check Border Router  TEST SKIP \n");
+        GREENTEA_TESTSUITE_RESULT(false);
+    }
+#endif


This same code block has now been copied on many places. Can it me just one function?

Perhaps we could follow the way this is done with a function+macro for SKIP_IF_TCP_UNSUPPORTED?

Actually, @tymoteuszblochmobica pointed out to me, that this is already only copied once per test directory. We can wrap it with a function, but we'll have to declare and define this function for every test directory (tcp, tls, dns, udp) anyway. We've been through similar discussion in #10037 and we agreed that there is no function sharing between different test suites, unfortunately.

So I'm afraid this cannot be just one function.

SeppoTakalo · 2019-10-09T06:10:02Z

TESTS/netsocket/tcp/main.cpp

+#if MBED_CONF_TARGET_NETWORK_DEFAULT_INTERFACE_TYPE != MESH
    Case("TCPSOCKET_RECV_100K", TCPSOCKET_RECV_100K),
    Case("TCPSOCKET_RECV_100K_NONBLOCK", TCPSOCKET_RECV_100K_NONBLOCK),
+#endif


I don't understand why this makes sense?

Does it really help if we start skipping test cases?

Its a big challenge to pass 100k recv in mesh.

After increasing timeouts up to 20 minutes tests mostly pass but still sometimes fails.
Smaller timeouts causes 100% fails

Its rfc864 tests so receiving DUT cant slow down transmision if mesh network is unable to deliver packets on time.

So problem with TCP retransmision traffic will arise.

Maybe the solution it to keep this increased timeouts for mesh and run tests only in RF box under no other devices "RF traffic "and "clear channel" condition.

Test skip reverted. It is successful under local mesh BR. Testing mesh performance under heavy RF traffic is beyond the scope of netsockets tests.

SeppoTakalo · 2019-10-09T06:11:27Z

TESTS/netsocket/udp/main.cpp

+#if MBED_CONF_TARGET_NETWORK_DEFAULT_INTERFACE_TYPE != MESH
    Case("UDPSOCKET_ECHOTEST_BURST_NONBLOCK", UDPSOCKET_ECHOTEST_BURST_NONBLOCK),
+#endif


This is something we should not skip.
Its basically close to what DTLS handshake is sending. Not accurate, but still emulates it.
Now we are just hiding problems.

Test skip reverted. It is successful under local mesh BR. Testing mesh performance under heavy RF traffic is beyond the scope of netsockets tests.

SeppoTakalo · 2019-10-09T06:11:52Z

TESTS/netsocket/udp/main.cpp

+#if MBED_CONF_TARGET_NETWORK_DEFAULT_INTERFACE_TYPE != MESH
    Case("UDPSOCKET_ECHOTEST_BURST", UDPSOCKET_ECHOTEST_BURST),
+#endif


Same here.. I don't see the point of skipping this.

Test skip reverted. It is successful under local mesh BR. Testing mesh performance under heavy RF traffic is beyond the scope of netsockets tests.

SeppoTakalo · 2019-10-09T06:12:39Z

TESTS/netsocket/udp/udpsocket_echotest.cpp

+static const int pkt_sizes[PKTS] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, \
+                                    100, 200, 300
+                                   };


Then who tests that those bigger packets can pass if we are not going to?

Test skip reverted. It is successful under local mesh BR. Testing mesh performance under heavy RF traffic is beyond the scope of netsockets tests.

0xc0170 · 2019-10-18T13:02:18Z

@SeppoTakalo @tymoteuszblochmobica What is the status for this PR? Looks like the review has not been completed

tymoteuszblochmobica · 2019-10-28T13:02:10Z

Seppo can you review again?

AnttiKauppila · 2019-11-01T13:09:19Z

TESTS/netsocket/tcp/main.cpp

+    uint8_t addr_hex[16];
+    stoip6(address, strlen(address), addr_hex);
+    if ((addr_hex[0] & 0xe0) != 0x20) {
+        printf("Adress is not globlal  Please check Border Router  TEST SKIP \n");


Please fix typos adress -> Address and globlal -> global (+ 2 other files where you have copy pasted same mistake)

0xc0170 · 2019-11-06T14:52:07Z

CI started

mbed-ci · 2019-11-06T17:29:01Z

Test run: FAILED

Summary: 1 of 5 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

michalpasztamobica · 2019-11-07T07:13:44Z

@tymoteuszblochmobica , apparently your check if the test is running for mesh network didn't work, although I can't see any obvious problem with it.
The logs all say:

[1573059766.57][CONN][RXD] MBED: IP address is '192.168.64.108'
[1573059766.76][CONN][RXD] Address is not global Please check Border Router TEST SKIP

I doubt that our CI is running any mesh network tests. This shouldn't trigger.

0xc0170 · 2019-11-07T08:23:09Z

@tymoteuszblochmobica Please talk to @VeliMattiLahtela

tymoteuszblochmobica · 2019-11-07T17:32:10Z

Strange
Found in NetworkInterfaceDefaults.cpp
/* Finally the dispatch from the JSON default interface type to the specific

subclasses. It's our job to configure - the default NetworkInterface is
preconfigured - the specific subtypes' defaults are not (necessarily).
*/
#define ETHERNET 1
#define WIFI 2
#define MESH 3
#define CELLULAR 4
#if MBED_CONF_TARGET_NETWORK_DEFAULT_INTERFACE_TYPE == 3 or prvevious
#define MESH 3
in main.cpp for each test is working.

SeppoTakalo · 2019-11-07T17:56:45Z

What strange there is about that?
This is how default network interface is selected.

For example test configuration on mesh, see: https://github.com/ARMmbed/mbed-os/blob/master/tools/test_configs/6lowpanInterface_router.json#L26..L36

TESTS/netsocket/dns/main.cpp

SeppoTakalo · 2019-11-07T18:04:15Z

TESTS/netsocket/tls/main.cpp

+    stoip6(address, strlen(address), addr_hex);
+    if ((addr_hex[0] & 0xe0) != 0x20) {
+        printf("Address is not global  Please check Border Router  TEST SKIP \n");
+        GREENTEA_TESTSUITE_RESULT(false);


Should this be TEST_SKIP_MESSAGE("Mesh not working")

I don't really know the difference why

I don't think we want to skip the tests? We just want them to fail with a reasonable message, pointing to the border router issue, to avoid unnecessary debugging/bug reports.

tymoteuszblochmobica · 2019-11-08T07:33:24Z

Fixed

mikter · 2019-11-08T09:51:18Z

TESTS/netsocket/tcp/main.cpp

+#if MBED_CONF_TARGET_NETWORK_DEFAULT_INTERFACE_TYPE == MESH
+    uint8_t addr_hex[16];
+    stoip6(address, strlen(address), addr_hex);
+    if ((addr_hex[0] & 0xe0) != 0x20) {


what are these why is there a check for address for mesh

To me this looks like bug in code that is circumvented in tests?

In applications we should wait for Global UP event and then everything should work

@mikter , this PR is now only trying to improve error reporting due to mesh network in RAAS being heavily loaded and often unable to perform UPD tests. We can tell that this situation occurs by observing the IP address our DUT got. If we did not receive a global address, the test will fail anyway, so we just want to minimize the amount of unnecessary debugging and print a reasonable error message suggesting

Now that you mentioned it, perhaps it is neater to wait for GLOBAL_UP event instead of checking if the address is global or not, but the essence of the changes remains the same - don't run the tests if the global connection failed and print a reasonable error message, instead.

@tymoteuszblochmobica , can we try observing the events instead of observing the IP address? I imagine we could set a blocking semaphore after connect() returns and only release it in a GLOBAL_UP event callback. If the semaphore is not released in time, the test will timeout. If the last message printed is "Waiting for GLOBAL_UP" I think that will be a clear indication that there is a connectivity problem. Does this make sense?

Replaced with GLOBAL_UP wait.

mikter

Is this test checking that address is 20**** meaning some default public IPv6 address and if not skip it.

This should be fixed in mesh API if there is a bug that ULA prefix is global

mikter · 2019-11-08T10:25:11Z

If you check file meshinterfacenanostack or any other mesh there is a check when local or Global is UP and there Global UP is given even for ULA prefixes

How is mesh.connect supposed to work wait until GLOBAL_UP or LOCAL_UP. if it should wait for GLOBAL_UP the bug is in the mesh apis

if it should stop even at LOCAL_UP the bug is in test case and it should follow the same check as mesh_minimal_application and do following while loop where it waits the Global Address. not failure

michalpasztamobica · 2019-11-08T13:02:55Z

Regarding what connect() should do... The connect() function will try to bringup() a particular interface. The bringup() implementations (Thread, for example) will block on the connect_semaphore, which is released in Nanostack::Interface::network_handler whenever the status is either CONNECTED, LOCAL or GLOBAL. So any of them means that connect() succeeded. This makes sense to me. The network may not always require global connectivity. This is also in line with @kjbracey-arm 's comment in #11714 - a local IPv6 address is perfectly valid, so to me connect() should return when local connectivity is present.

I agree with you, @mikter , that it is better to wait for GLOBAL_UP event until test times out. @tymoteuszblochmobica , do you also agree? Can you modify the code so tests wait until GLOBAL_UP is present?

kjbracey · 2019-11-08T13:10:37Z

There's no clearly right behaviour here.

It's similar to lwIP's dilemma about what do do when asked for IPv4+IPv6, preferring IPv6, and it gets IPv4 first. How long should "connect" wait for IPv6 before returning? Should it wait, even?

There we did decide to make it wait a bit longer to see if IPv6 came up, on the assumption that apps might just be written to do a simple "connect" then check "IPv4 or IPv6 address"?

You could do the same thing here - wait a bit longer after the local up, to see if we get a global address, for simple apps that are going to check the address immediately after connect return. It would be consistent.

tymoteuszblochmobica · 2019-11-08T18:28:58Z

Replaced with GLOBAL_UP waiting loop.
@mikter Is it OK now?

0xc0170 · 2019-11-15T17:51:55Z

@mikter Please review

tymoteuszblochmobica · 2019-11-20T11:14:13Z

Rebased

michalpasztamobica

@0xc0170 , it seems that this is now also ready to go (as soon as the current release is over). Would you please adjust the labels?

0xc0170 · 2019-11-20T13:21:27Z

CI started

mbed-ci · 2019-11-20T14:42:12Z

Test run: SUCCESS

Summary: 5 of 5 test jobs passed
Build number : 2
Build artifacts

tymoteuszblochmobica force-pushed the mesh branch from d5cfc7b to 34c3302 Compare October 8, 2019 15:26

ciarmcom requested review from michalpasztamobica, mikter, SeppoTakalo and a team October 8, 2019 17:00

ciarmcom added the needs: review label Oct 8, 2019

SeppoTakalo reviewed Oct 9, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the mesh branch 2 times, most recently from 472d254 to 512e237 Compare October 28, 2019 12:59

tymoteuszblochmobica force-pushed the mesh branch 2 times, most recently from 3703305 to 7c8abc5 Compare October 28, 2019 13:05

AnttiKauppila suggested changes Nov 1, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the mesh branch from 7c8abc5 to 1d6985b Compare November 4, 2019 18:40

AnttiKauppila approved these changes Nov 6, 2019

View reviewed changes

AnttiKauppila added needs: CI and removed needs: review labels Nov 6, 2019

0xc0170 approved these changes Nov 6, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Nov 7, 2019

SeppoTakalo reviewed Nov 7, 2019

View reviewed changes

TESTS/netsocket/dns/main.cpp Show resolved Hide resolved

SeppoTakalo reviewed Nov 7, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the mesh branch from 1d6985b to 6d6076d Compare November 7, 2019 18:27

tymoteuszblochmobica force-pushed the mesh branch from 6d6076d to b6b4519 Compare November 8, 2019 08:11

mikter reviewed Nov 8, 2019

View reviewed changes

mikter requested changes Nov 8, 2019

View reviewed changes

tymoteuszblochmobica force-pushed the mesh branch from b6b4519 to 1aacaf3 Compare November 8, 2019 18:25

0xc0170 requested a review from mikter November 15, 2019 17:52

0xc0170 added needs: review and removed needs: work labels Nov 15, 2019

Adjustment of netsocket tests to the lossy mesh network limitation.

983c958

tymoteuszblochmobica force-pushed the mesh branch from 1aacaf3 to 983c958 Compare November 20, 2019 11:14

mikter approved these changes Nov 20, 2019

View reviewed changes

michalpasztamobica approved these changes Nov 20, 2019

View reviewed changes

0xc0170 added needs: CI release-version: 5.15.0-rc1 and removed needs: review labels Nov 20, 2019

0xc0170 removed the needs: CI label Nov 20, 2019

0xc0170 merged commit 48f38a4 into ARMmbed:master Nov 20, 2019

Adjustment of netsocket tests to the lossy mesh network limitation. #11654

Adjustment of netsocket tests to the lossy mesh network limitation. #11654

Uh oh!

Conversation

tymoteuszblochmobica commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Reviewers

Release Notes

Uh oh!

ciarmcom commented Oct 8, 2019

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!

tymoteuszblochmobica Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

0xc0170 commented Oct 18, 2019

Uh oh!

tymoteuszblochmobica commented Oct 28, 2019

Uh oh!

AnttiKauppila Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 6, 2019

Uh oh!

mbed-ci commented Nov 6, 2019

Uh oh!

michalpasztamobica commented Nov 7, 2019

Uh oh!

0xc0170 commented Nov 7, 2019

Uh oh!

tymoteuszblochmobica commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeppoTakalo commented Nov 7, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tymoteuszblochmobica commented Nov 8, 2019

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!

mikter left a comment

Choose a reason for hiding this comment

Uh oh!

mikter commented Nov 8, 2019

Uh oh!

tymoteuszblochmobica commented Oct 8, 2019 •

edited

Loading

tymoteuszblochmobica Oct 11, 2019 •

edited

Loading

AnttiKauppila Nov 1, 2019 •

edited

Loading

tymoteuszblochmobica commented Nov 7, 2019 •

edited

Loading