From aad9844496cdfecdcb7ffcaab27b8941cf5e0458 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Tue, 14 Mar 2023 16:55:05 -0700 Subject: [PATCH] Attempt to fix NetworkTracePollerTest flake The test has flaked twice, once reporting only half of the packets, the second time reporting no packets. Although I haven't been able to repro this locally with 500 attempts, I believe it's a race between the kernel thread writing and test thread receiving the events. This adds a retry with a brief sleep only if we don't get all packets on the first try. This also records all unmatched packets in case something like the port or uid reporting breaks. Rather than fail saying zero packets, you'll see the ones we skipped and can (hopefully) tell what went wrong. Bug: 273600719 Test: atest libnetworkstats_test Change-Id: Iee21f30a8dc59be5649f8e8b6509f4cc69ae5ff9 --- .../NetworkTracePollerTest.cpp | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTracePollerTest.cpp b/service-t/native/libs/libnetworkstats/NetworkTracePollerTest.cpp index 725cec1168..df07bbe453 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTracePollerTest.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTracePollerTest.cpp @@ -26,6 +26,8 @@ #include #include +#include +#include #include #include "netdbpf/NetworkTracePoller.h" @@ -130,16 +132,21 @@ TEST_F(NetworkTracePollerTest, ConcurrentSessions) { TEST_F(NetworkTracePollerTest, TraceTcpSession) { __be16 server_port = 0; - std::vector packets; + std::vector packets, unmatched; // Record all packets with the bound address and current uid. This callback is // involked only within ConsumeAll, at which point the port should have // already been filled in and all packets have been processed. NetworkTracePoller handler([&](const std::vector& pkts) { for (const PacketTrace& pkt : pkts) { - if (pkt.sport != server_port && pkt.dport != server_port) return; - if (pkt.uid != getuid()) return; - packets.push_back(pkt); + if ((pkt.sport == server_port || pkt.dport == server_port) && + pkt.uid == getuid()) { + packets.push_back(pkt); + } else { + // There may be spurious packets not caused by the test. These are only + // captured so that we can report them to help debug certain errors. + unmatched.push_back(pkt); + } } }); @@ -179,7 +186,13 @@ TEST_F(NetworkTracePollerTest, TraceTcpSession) { EXPECT_EQ(std::string(data), std::string(buff)); } - ASSERT_TRUE(handler.ConsumeAll()); + // Poll until we get all the packets (typically we get it first try). + for (int attempt = 0; attempt < 10; attempt++) { + ASSERT_TRUE(handler.ConsumeAll()); + if (packets.size() >= 12) break; + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + ASSERT_TRUE(handler.Stop()); // There are 12 packets in total (6 messages: each seen by client & server): @@ -189,7 +202,9 @@ TEST_F(NetworkTracePollerTest, TraceTcpSession) { // 4. Client sends data with psh ack // 5. Server acks the data packet // 6. Client closes connection with fin ack - ASSERT_EQ(packets.size(), 12) << PacketPrinter{packets}; + ASSERT_EQ(packets.size(), 12) + << PacketPrinter{packets} + << "\nUnmatched packets: " << PacketPrinter{unmatched}; // All packets should be TCP packets. EXPECT_THAT(packets, Each(Field(&PacketTrace::ipProto, Eq(IPPROTO_TCP))));