From 216b740663bd897dad177b57953c59860105020d Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Tue, 17 Aug 2021 23:35:10 -0700 Subject: [op] Deflake SNMP Metric proc_net tests. Earlier the tests were checking for equality of system-wide metrics before and after some network related operations. That is inherently racy for native tests because depending on the testing infrastructure, multiple tests might run parallely hence trampling over each other's metrics. Tests should only compare metrics that are increasing in nature. The comparison should not be a hard comparison, instead a less-than/greater-than relation test. I have changed the checks and also removed tests for tcpCurrEstab metric which has "SYNTAX Gauge" and hence can not be tested reliably. PiperOrigin-RevId: 391460081 --- test/syscalls/linux/proc_net.cc | 61 +++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 20 deletions(-) (limited to 'test') diff --git a/test/syscalls/linux/proc_net.cc b/test/syscalls/linux/proc_net.cc index 162c0b665..672f2dd42 100644 --- a/test/syscalls/linux/proc_net.cc +++ b/test/syscalls/linux/proc_net.cc @@ -168,6 +168,21 @@ int GetMibsAllocationSysctl() { return val; } +// GetSNMPMetricFromProc retrieves the metric named `item` of `type` from the +// `snmp` string which represents the file contents of /proc/net/snmp. +// +// Note to test writers: If you are writing tests to check the change in value +// of SNMP metrics, note that if GetMibsAllocationSysctl() == 0 +// (net.core.mibs_allocation isn't set), MIB is from the init network namespace +// and hence these metrics can have system-wide noise. +// +// A feasible way of testing is the following: +// * Consult RFC 1213 to learn the "SYNTAX" of the metric. +// * If net.core.mibs_allocation is set: +// - Can test any metric while checking for hard equality. +// * If net.core.mibs_allocation is NOT set (system-wide noise is present): +// - Only test "SYNTAX Counter" metrics. +// - Counter metrics are increasing in nature, so use LE/GE equality checks. PosixErrorOr GetSNMPMetricFromProc(const std::string snmp, const std::string& type, const std::string& item) { @@ -243,18 +258,22 @@ TEST(ProcNetSnmp, TcpReset) { GetSNMPMetricFromProc(snmp, "Tcp", "AttemptFails")); if (GetMibsAllocationSysctl()) { - EXPECT_EQ(oldActiveOpens, newActiveOpens - 1); - EXPECT_EQ(oldOutRsts, newOutRsts - 1); - EXPECT_EQ(oldAttemptFails, newAttemptFails - 1); + EXPECT_EQ(oldActiveOpens + 1, newActiveOpens); + EXPECT_EQ(oldOutRsts + 1, newOutRsts); + EXPECT_EQ(oldAttemptFails + 1, newAttemptFails); } else { - // System-wide statistics can have some noise. - EXPECT_LE(oldOutRsts, newOutRsts - 1); - EXPECT_LE(oldAttemptFails, newAttemptFails - 1); + // System-wide statistics can have some noise. These metrics should have + // increased by at least 1. + EXPECT_LE(oldActiveOpens + 1, newActiveOpens); + EXPECT_LE(oldOutRsts + 1, newOutRsts); + EXPECT_LE(oldAttemptFails + 1, newAttemptFails); } } TEST(ProcNetSnmp, TcpEstab) { - // System-wide statistics can have some noise. + // This test aims to test the tcpCurrEstab metric which has "SYNTAX Gauge" as + // per RFC 1213. Hence, it becomes infeasible to test this when system-wide + // statistics have noise. SKIP_IF(GetMibsAllocationSysctl() == 0); // TODO(gvisor.dev/issue/866): epsocket metrics are not savable. @@ -311,9 +330,9 @@ TEST(ProcNetSnmp, TcpEstab) { newCurrEstab = ASSERT_NO_ERRNO_AND_VALUE( GetSNMPMetricFromProc(snmp, "Tcp", "CurrEstab")); - EXPECT_EQ(oldActiveOpens, newActiveOpens - 1); - EXPECT_EQ(oldPassiveOpens, newPassiveOpens - 1); - EXPECT_EQ(oldCurrEstab, newCurrEstab - 2); + EXPECT_EQ(oldActiveOpens + 1, newActiveOpens); + EXPECT_EQ(oldPassiveOpens + 1, newPassiveOpens); + EXPECT_EQ(oldCurrEstab + 2, newCurrEstab); // Send 1 byte from client to server. ASSERT_THAT(send(s_connect.get(), "a", 1, 0), SyscallSucceedsWithValue(1)); @@ -381,12 +400,13 @@ TEST(ProcNetSnmp, UdpNoPorts) { ASSERT_NO_ERRNO_AND_VALUE(GetSNMPMetricFromProc(snmp, "Udp", "NoPorts")); if (GetMibsAllocationSysctl()) { - EXPECT_EQ(oldOutDatagrams, newOutDatagrams - 1); - EXPECT_EQ(oldNoPorts, newNoPorts - 1); + EXPECT_EQ(oldOutDatagrams + 1, newOutDatagrams); + EXPECT_EQ(oldNoPorts + 1, newNoPorts); } else { - // System-wide statistics can have some noise. - EXPECT_LE(oldOutDatagrams, newOutDatagrams - 1); - EXPECT_LE(oldNoPorts, newNoPorts - 1); + // System-wide statistics can have some noise. These metrics should have + // increased by at least 1. + EXPECT_LE(oldOutDatagrams + 1, newOutDatagrams); + EXPECT_LE(oldNoPorts + 1, newNoPorts); } } @@ -437,12 +457,13 @@ TEST(ProcNetSnmp, UdpIn) { GetSNMPMetricFromProc(snmp, "Udp", "InDatagrams")); if (GetMibsAllocationSysctl()) { - EXPECT_EQ(oldOutDatagrams, newOutDatagrams - 1); - EXPECT_EQ(oldInDatagrams, newInDatagrams - 1); + EXPECT_EQ(oldOutDatagrams + 1, newOutDatagrams); + EXPECT_EQ(oldInDatagrams + 1, newInDatagrams); } else { - // System-wide statistics can have some noise. - EXPECT_LE(oldOutDatagrams, newOutDatagrams - 1); - EXPECT_LE(oldInDatagrams, newInDatagrams - 1); + // System-wide statistics can have some noise. These metrics should have + // increased by at least 1. + EXPECT_LE(oldOutDatagrams + 1, newOutDatagrams); + EXPECT_LE(oldInDatagrams + 1, newInDatagrams); } } -- cgit v1.2.3