Flaky test: TestPeersTotal

[aschmahmann]

https://app.circleci.com/pipelines/github/ipfs/go-ipfs/4610/workflows/d1451a93-9637-482c-b9e1-f1914dfaa687/jobs/51152/tests#failed-test-0

=== RUN   TestPeersTotal
2021/05/12 20:33:38 failed to sufficiently increase receive buffer size (was: 208 kiB, wanted: 2048 kiB, got: 416 kiB). See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details.
    TestPeersTotal: metrics_test.go:45: expected 1 peers transport, got 2
--- FAIL: TestPeersTotal (0.16s)

After re-running it is green, so filing this issue so we don't forget to fix this).

[masih]

Looking at CircleCi config, it looks like gotest job runs on Docker. To set sysctl we need to run that on a machine executor; See docs.

Running that job on machine will have longer start-up time, and will potentially involve installing go manually.
@aschmahmann Do we want to make this change?

[aschmahmann]

@masih the UDP buffer size should be an independent problem from the flaky test, it's just a data transfer efficiency thing.

At the moment it doesn't seem worthwhile to change the setup to run as a machine, especially given that we'd like to move towards using GitHub Actions #8348

[schomatis]

The test on which this one is based

https://github.com/ipfs/go-ipfs/blob/ef0428af407410d7f7e98ec0b3e57afd8c828031/core/corehttp/metrics_test.go#L15

has been fixed for the same error in libp2p/go-libp2p-swarm@0538806.

@aschmahmann I'm not sure how to fix this as I don't know the original design rationale behind the test nor the function we are testing. The original fix changes its test to track peers instead of open connections (because of what's explained in the commit message) but in our function being tested we track connections and I imagine changing that to tracking peers instead would have unintended side effects in the callers consuming this information.

[BigLep]

Next step is for @petar to look to identify next step. We may not need this if this test is a copy from libp2p.

[petar]

I've added more logging to the test for now, so that next time it breaks we can understand why the connections are established over different transports.

[BigLep]

This is blocked until we find another occurrence of this so we can look at the more verbose logs.

@petar : I assume you haven't seen another instance?

[petar]

Nope. The logging code is checked into master. Waiting for the flakiness to occur again.

…

On Fri, 8 Oct 2021 at 08:16, Steve Loeppky ***@***.***> wrote: This is blocked until we find another occurrence of this so we can look at the more verbose logs. @petar <https://github.com/petar> : I assume you haven't seen another instance? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8135 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACFTS7YYZW37HGORK6Y77TUF4DLPANCNFSM442WZGXQ> .

[schomatis]

@petar Following up on this:

On the extra logging, we will normally dial both TCP and QUIC/UDP simultaneously and add both connections to the list, see the swarm dial worker logic for more details.

As mentioned above this is already acknowledged upstream and that is why libp2p/go-libp2p-swarm@0538806 no longer checks the list of connections but explicitly the list of peers in its own test (because one peer might have more than one active connection).

I think we should port this fix here as well, consulting the peer list instead of the connection list where appropriate. My main doubt is understanding the actual purpose of PeersTotalValues to decide how to best do that.

[petar]

@schomatis I agree. The ipfs test should use peer count, and not be dependent on the underlying implementation of swarm (which can vary the connection count per peer).

[schomatis]

Ok, will push something for this next week.

[BigLep]

Per 2021-11-12 verbal and slack: the test should be updated to assert on the number of peers (not the number of connections).

@galargh: if you have other comments/context from Slack, feel free to add.

[galargh]

Nothing to add, #8135 (comment) says it all.

[schomatis]

Picking this up again.