From 183ecd54fe9d7958dbbb679edba4d62747fc6d39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Szab=C3=B3?= Date: Mon, 29 Jul 2024 00:59:05 +0200 Subject: [PATCH] Fix tests --- .../network/test/AsyncMcClientTestSync.cpp | 62 ++++++++++++++++--- mcrouter/lib/network/test/CMakeLists.txt | 1 + mcrouter/lib/network/test/MockMc.cpp | 4 ++ mcrouter/lib/network/test/MockMc.h | 2 + mcrouter/lib/network/test/MockMcOnRequest.h | 7 +++ .../lib/network/test/TestClientServerUtil.cpp | 2 + mcrouter/test/MCProcess.py | 14 +++-- .../cpp_unit_tests/McrouterClientUsage.cpp | 1 + mcrouter/test/mcrouter_config.py | 5 ++ mcrouter/test/test_async_files_attr.py | 6 +- mcrouter/test/test_axonlog.py | 7 +++ mcrouter/test/test_carbonlookaside_route.py | 8 ++- mcrouter/test/test_mcrouter_basic.py | 7 ++- .../test/test_mcrouter_processing_time.py | 4 ++ mcrouter/test/test_mcrouter_sanity_mock.py | 5 ++ mcrouter/test/test_unknown_name_handle.json | 46 +++++++------- mcrouter/test/test_warmup2.json | 3 +- 17 files changed, 142 insertions(+), 42 deletions(-) diff --git a/mcrouter/lib/network/test/AsyncMcClientTestSync.cpp b/mcrouter/lib/network/test/AsyncMcClientTestSync.cpp index cd3278ce1..51804f9ea 100644 --- a/mcrouter/lib/network/test/AsyncMcClientTestSync.cpp +++ b/mcrouter/lib/network/test/AsyncMcClientTestSync.cpp @@ -5,6 +5,8 @@ * LICENSE file in the root directory of this source tree. */ +#include +#include #include #include @@ -869,6 +871,9 @@ TEST(AsyncMcClient, caretVersionUserSpecified) { } TEST(AsyncMcClient, caretAdditionalFields) { +#ifdef LIBMC_FBTRACE_DISABLE + GTEST_SKIP() << "Tracing is disabled in the OSS build"; +#endif TestServer::Config config; config.useSsl = false; auto server = TestServer::create(std::move(config)); @@ -958,6 +963,26 @@ using TFOTestParams = std::tuple; class AsyncMcClientTFOTest : public TestWithParam {}; +/** + * Check whether the current host system supports TCP fastopen. + */ +bool tfoSupportedOnHost() { + try { + constexpr int kClientSupport = 1; + constexpr int kDataInOpeningSynWithoutCookie = 4; + int flags; + std::ifstream procFs("/proc/sys/net/ipv4/tcp_fastopen"); + procFs >> flags; + + return + (flags & kClientSupport) == kClientSupport && + (flags & kDataInOpeningSynWithoutCookie) == kDataInOpeningSynWithoutCookie; + } catch (std::exception& e) { + LOG(ERROR) << "Could not read TCP fastopen sysctl: " << e.what(); + return false; + } +} + TEST_P(AsyncMcClientTFOTest, testTfoWithSSL) { auto serverEnabled = std::get<0>(GetParam()); auto clientEnabled = std::get<1>(GetParam()); @@ -971,9 +996,10 @@ TEST_P(AsyncMcClientTFOTest, testTfoWithSSL) { auto offloadHandshake = std::get<2>(GetParam()); auto constexpr nConnAttempts = 10; + auto tfoSupported = tfoSupportedOnHost(); auto mech = std::get<3>(GetParam()); - auto sendReq = [serverEnabled, clientEnabled, mech](TestClient& client) { + auto sendReq = [serverEnabled, clientEnabled, tfoSupported, mech](TestClient& client) { client.setConnectionStatusCallbacks( [&](const folly::AsyncTransportWrapper& sock, int64_t) { if (mech == SecurityMech::TLS_TO_PLAINTEXT) { @@ -982,10 +1008,16 @@ TEST_P(AsyncMcClientTFOTest, testTfoWithSSL) { auto stats = socket->getStats(); if (clientEnabled) { EXPECT_TRUE(stats.tfoAttempted); - EXPECT_TRUE(stats.tfoFinished); - // we can not guarantee socket->getTFOSucceeded() will return true - // unless there are specific kernel + host settings applied - if (!serverEnabled) { + + if (tfoSupported) { + EXPECT_TRUE(stats.tfoFinished); + if (serverEnabled) { + EXPECT_TRUE(stats.tfoSuccess); + } else { + EXPECT_FALSE(stats.tfoSuccess); + } + } else { + EXPECT_FALSE(stats.tfoFinished); EXPECT_FALSE(stats.tfoSuccess); } } else { @@ -995,10 +1027,16 @@ TEST_P(AsyncMcClientTFOTest, testTfoWithSSL) { auto* socket = sock.getUnderlyingTransport(); if (clientEnabled) { EXPECT_TRUE(socket->getTFOAttempted()); - EXPECT_TRUE(socket->getTFOFinished()); - // we can not guarantee socket->getTFOSucceeded() will return true - // unless there are specific kernel + host settings applied - if (!serverEnabled) { + + if (tfoSupported) { + EXPECT_TRUE(socket->getTFOFinished()); + if (serverEnabled) { + EXPECT_TRUE(socket->getTFOSucceded()); + } else { + EXPECT_FALSE(socket->getTFOSucceded()); + } + } else { + EXPECT_FALSE(socket->getTFOFinished()); EXPECT_FALSE(socket->getTFOSucceded()); } } else { @@ -1143,6 +1181,9 @@ TEST_P(AsyncMcClientSSLOffloadTest, closeNow) { EXPECT_FALSE(upCalled); EXPECT_TRUE(downReason.has_value()); EXPECT_EQ(*downReason, ConnectionDownReason::ABORTED); + + server->shutdown(); + server->join(); } TEST_P(AsyncMcClientSSLOffloadTest, clientReset) { @@ -1164,6 +1205,9 @@ TEST_P(AsyncMcClientSSLOffloadTest, clientReset) { evb.loopOnce(); client.reset(); evb.loop(); + + server->shutdown(); + server->join(); } INSTANTIATE_TEST_CASE_P(AsyncMcClientTest, AsyncMcClientSSLOffloadTest, Bool()); diff --git a/mcrouter/lib/network/test/CMakeLists.txt b/mcrouter/lib/network/test/CMakeLists.txt index 3eac54030..6d7c0ec7b 100644 --- a/mcrouter/lib/network/test/CMakeLists.txt +++ b/mcrouter/lib/network/test/CMakeLists.txt @@ -30,6 +30,7 @@ target_link_libraries( FBThrift::thrift-core) add_executable(mcrouter_mock_mc_thrift_server MockMc.cpp MockMcThriftServer.cpp) +set_property(TARGET mcrouter_mock_mc_thrift_server PROPERTY OUTPUT_NAME mock_mc_thrift_server) target_link_libraries( mcrouter_mock_mc_thrift_server diff --git a/mcrouter/lib/network/test/MockMc.cpp b/mcrouter/lib/network/test/MockMc.cpp index cf87e8704..bf31a0c4b 100644 --- a/mcrouter/lib/network/test/MockMc.cpp +++ b/mcrouter/lib/network/test/MockMc.cpp @@ -245,6 +245,10 @@ MockMc::findUnexpired(folly::StringPiece key) { return it; } +size_t MockMc::itemCount() const noexcept { + return citems_.size(); +} + void MockMc::flushAll() { citems_.clear(); } diff --git a/mcrouter/lib/network/test/MockMc.h b/mcrouter/lib/network/test/MockMc.h index 8d35019ef..52ecaea25 100644 --- a/mcrouter/lib/network/test/MockMc.h +++ b/mcrouter/lib/network/test/MockMc.h @@ -146,6 +146,8 @@ class MockMc { CasResult cas(folly::StringPiece key, Item item, uint64_t token); + size_t itemCount() const noexcept; + /** * clear all items */ diff --git a/mcrouter/lib/network/test/MockMcOnRequest.h b/mcrouter/lib/network/test/MockMcOnRequest.h index ba24c08eb..dbc4b140b 100644 --- a/mcrouter/lib/network/test/MockMcOnRequest.h +++ b/mcrouter/lib/network/test/MockMcOnRequest.h @@ -19,6 +19,8 @@ namespace facebook { namespace memcache { static std::atomic_uint64_t cmd_get_count{0}; +static std::atomic_uint64_t cmd_lease_get_count{0}; +static std::atomic_uint64_t cmd_lease_set_count{0}; class MockMcOnRequest { public: @@ -127,6 +129,7 @@ class MockMcOnRequest { template void onRequest(Context&& ctx, McLeaseGetRequest&& req) { using Reply = McLeaseGetReply; + ++cmd_lease_get_count; auto key = req.key_ref()->fullKey().str(); @@ -144,6 +147,7 @@ class MockMcOnRequest { template void onRequest(Context&& ctx, McLeaseSetRequest&& req) { using Reply = McLeaseSetReply; + ++cmd_lease_set_count; auto key = req.key_ref()->fullKey().str(); @@ -355,6 +359,9 @@ class MockMcOnRequest { if (key == "__mockmc__") { StatsReply stats; stats.addStat("cmd_get_count", cmd_get_count.load()); + stats.addStat("cmd_lease_get", cmd_lease_get_count.load()); + stats.addStat("cmd_lease_set", cmd_lease_set_count.load()); + stats.addStat("total_items", mc_.itemCount()); Context::reply(std::move(ctx), stats.getReply()); } else { Context::reply(std::move(ctx), Reply(carbon::Result::BAD_COMMAND)); diff --git a/mcrouter/lib/network/test/TestClientServerUtil.cpp b/mcrouter/lib/network/test/TestClientServerUtil.cpp index 5722a1457..e09b8445f 100644 --- a/mcrouter/lib/network/test/TestClientServerUtil.cpp +++ b/mcrouter/lib/network/test/TestClientServerUtil.cpp @@ -87,6 +87,8 @@ void TestServerOnRequest::onRequest( processReply(std::move(ctx), Reply(carbon::Result::NOTFOUND)); } else if (req.key_ref()->fullKey() == "shutdown") { shutdownLock_.post(); + // TODO + std::this_thread::sleep_for(std::chrono::milliseconds(100)); processReply(std::move(ctx), Reply(carbon::Result::NOTFOUND)); flushQueue(); } else if (req.key_ref()->fullKey() == "busy") { diff --git a/mcrouter/test/MCProcess.py b/mcrouter/test/MCProcess.py index 5ba44938e..5ce0a533c 100644 --- a/mcrouter/test/MCProcess.py +++ b/mcrouter/test/MCProcess.py @@ -591,7 +591,6 @@ def stats(self, spec=None): if spec: q = "stats {spec}\r\n".format(spec=spec) self._sendall(q) - s = {} line = None fds = select.select([self.fd], [], [], 20.0) @@ -800,13 +799,14 @@ def __init__(self, args, port=None, base_dir=None): self.stats_dir, "--debug-fifo-root", self.debug_fifo_root, - "--rss-limit-mb", - "16384", "--fibers-stack-size", "65536", ] ) + if not McrouterGlobals.ossVersion(): + args.extend(["--rss-limit-mb", "16384"]) + listen_sock = None pass_fds = [] if port is None: @@ -1030,7 +1030,8 @@ def __init__(self, port=None, ssl_port=None, extra_args=None): pass_fds = [] # if mockmc is used here, we initialize the same way as MockMemcached - if McrouterGlobals.binPath("mockmc") == args[0]: + self.is_mock_server = McrouterGlobals.binPath("mockmc") == args[0] + if self.is_mock_server: if port is None: listen_sock = create_listen_socket() port = listen_sock.getsockname()[1] @@ -1103,6 +1104,11 @@ def __init__(self, port=None, ssl_port=None, extra_args=None): tries -= 1 self.disconnect() + def stats(self, spec=None): + if self.is_mock_server: + return super().stats('__mockmc__') + return super().stats(spec) + def getsslport(self): return self.ssl_port diff --git a/mcrouter/test/cpp_unit_tests/McrouterClientUsage.cpp b/mcrouter/test/cpp_unit_tests/McrouterClientUsage.cpp index f655bfefa..1384b19e5 100644 --- a/mcrouter/test/cpp_unit_tests/McrouterClientUsage.cpp +++ b/mcrouter/test/cpp_unit_tests/McrouterClientUsage.cpp @@ -116,6 +116,7 @@ TEST(CarbonRouterClient, basicUsageSameThreadClient) { // gracefully. This ensures graceful destruction of the static // CarbonRouterInstance instance. router->shutdown(); + ioThreadPool->join(); ioThreadPool.reset(); EXPECT_TRUE(replyReceived); } diff --git a/mcrouter/test/mcrouter_config.py b/mcrouter/test/mcrouter_config.py index 560a7355d..2ada6e0bf 100644 --- a/mcrouter/test/mcrouter_config.py +++ b/mcrouter/test/mcrouter_config.py @@ -21,6 +21,7 @@ def binPath(name): "mcrouter": "./mcrouter/mcrouter", "mcpiper": "./mcrouter/tools/mcpiper/mcpiper", "mockmc": "./mcrouter/lib/network/test/mock_mc_server", + "mockmcthrift": "./mcrouter/lib/network/test/mock_mc_thrift_server", "mockmcdual": "./mcrouter/lib/network/test/mock_mc_server_dual", "prodmc": "./mcrouter/lib/network/test/mock_mc_server", } @@ -34,6 +35,10 @@ def preprocessArgs(args): def useThriftClient(): return False + @staticmethod + def ossVersion() -> bool: + return True + @staticmethod def createThriftTestClient(addr, port) -> ThriftTestClient: raise NotImplementedError diff --git a/mcrouter/test/test_async_files_attr.py b/mcrouter/test/test_async_files_attr.py index 033766215..72470c858 100644 --- a/mcrouter/test/test_async_files_attr.py +++ b/mcrouter/test/test_async_files_attr.py @@ -9,9 +9,8 @@ import os import time -from libfb.py import parutil from mcrouter.test.McrouterTestCase import McrouterTestCase - +from mcrouter.test.config import McrouterGlobals class TestAsyncFilesAttr(McrouterTestCase): stat_prefix = "libmcrouter.mcrouter.0." @@ -53,6 +52,9 @@ def test_stats_no_requests(self): self.check_stats(mcrouter.stats_dir) def test_async_files_attr(self): + if McrouterGlobals.ossVersion(): + self.skipTest("The mcrouter client is not available in the OSS build") + from libfb.py import parutil mcrouter = self.add_mcrouter(self.config, extra_args=self.extra_args) binary = parutil.get_file_path("mcrouter/client_binary") port = str(mcrouter.getport()) diff --git a/mcrouter/test/test_axonlog.py b/mcrouter/test/test_axonlog.py index f7ee82023..c542a5eb8 100644 --- a/mcrouter/test/test_axonlog.py +++ b/mcrouter/test/test_axonlog.py @@ -5,6 +5,7 @@ # LICENSE file in the root directory of this source tree. from mcrouter.test.McrouterTestCase import McrouterTestCase +from mcrouter.test.config import McrouterGlobals from mcrouter.test.mock_servers import DeadServer @@ -19,6 +20,9 @@ def setUp(self): class TestAxonProxyFailedDelete(TestAxonLogBase): def setUp(self): + if McrouterGlobals.ossVersion(): + self.skipTest("Axon logging is not available in the OSS build") + self.mc1 = self.add_server(DeadServer(5000)) self.mr = self.add_mcrouter(self.config, extra_args=self.extra_args) @@ -43,6 +47,9 @@ class TestAxonLogAllDelete(TestAxonLogBase): config = "./mcrouter/test/test_axon_log_alldelete.json" def setUp(self): + if McrouterGlobals.ossVersion(): + self.skipTest("Axon logging is not available in the OSS build") + self.mr = self.add_mcrouter(self.config, extra_args=self.extra_args) def test_all_delete(self): diff --git a/mcrouter/test/test_carbonlookaside_route.py b/mcrouter/test/test_carbonlookaside_route.py index 91f1f3531..1af787a6f 100644 --- a/mcrouter/test/test_carbonlookaside_route.py +++ b/mcrouter/test/test_carbonlookaside_route.py @@ -127,7 +127,7 @@ def __init__( "TEMPLATE_PREFIX": prefix, "TEMPLATE_TTL": ttl, "TEMPLATE_TTL_UNIT_MS": has_ms_ttl, - "TEMPLATE_FILENAME": "file:" + self.tmpFlavorFile.name, + "TEMPLATE_FILENAME": self.tmpFlavorFile.name, "TEMPLATE_LEASE_ENABLE": lease_enable, "TEMPLATE_LEASE_WAIT_INTERVAL": lease_interval, "TEMPLATE_LEASE_NUM_RETRIES": lease_num_retries, @@ -159,6 +159,7 @@ def setUp(self): self.mcrouter = self.add_mcrouter(self.config, extra_args=self.extra_args) def tearDown(self): + super().tearDown() self.tmpConfig.cleanup() def test_carbonlookaside_basic(self): @@ -220,6 +221,7 @@ def setUp(self): self.mcrouter = self.add_mcrouter(self.config, extra_args=self.extra_args) def tearDown(self): + super().tearDown() self.tmpConfig.cleanup() def test_carbonlookaside_ttl_expiry(self): @@ -297,6 +299,7 @@ def setUp(self): self.mcrouter = self.add_mcrouter(self.config, extra_args=self.extra_args) def tearDown(self): + super().tearDown() self.tmpConfig.cleanup() def test_carbonlookaside_basic_leases(self): @@ -360,6 +363,7 @@ def setUp(self): self.mcrouter2 = self.add_mcrouter(self.config, extra_args=self.extra_args) def tearDown(self): + super().tearDown() self.tmpConfig.cleanup() def async_get(self, key, ret): @@ -425,6 +429,7 @@ def setUp(self): self.mcrouter = self.add_mcrouter(self.config, extra_args=self.extra_args) def tearDown(self): + super().tearDown() self.tmpConfig.cleanup() def test_carbonlookaside_ttl_ms_expiry_config(self): @@ -465,6 +470,7 @@ def setUp(self): self.mcrouter = self.add_mcrouter(self.config, extra_args=self.extra_args) def tearDown(self): + super().tearDown() self.tmpConfig.cleanup() def test_carbonlookaside_ttl_ms_expiry(self): diff --git a/mcrouter/test/test_mcrouter_basic.py b/mcrouter/test/test_mcrouter_basic.py index a13e6cced..70e21a366 100644 --- a/mcrouter/test/test_mcrouter_basic.py +++ b/mcrouter/test/test_mcrouter_basic.py @@ -8,7 +8,7 @@ import time from threading import Thread -from mcrouter.test.MCProcess import Mcrouter, McrouterClient, Memcached +from mcrouter.test.MCProcess import Mcrouter, McrouterClient, Memcached, BaseDirectory from mcrouter.test.McrouterTestCase import McrouterTestCase @@ -1137,7 +1137,10 @@ class TestMcrouterWithRetries(McrouterTestCase): invalid_config_with_retries = ( "./mcrouter/test/test_basic_l1_l2_sizesplit_retry_invalid.json" ) - extra_args = ["--validate-config=run"] + + def setUp(self): + self.config_dump_root = BaseDirectory("config_dump") + self.extra_args = ["--validate-config=run", "--config-dump-root", self.config_dump_root.path] def test_valid_retries(self): mcr = self.add_mcrouter( diff --git a/mcrouter/test/test_mcrouter_processing_time.py b/mcrouter/test/test_mcrouter_processing_time.py index 8cbf664ae..a6fa76223 100644 --- a/mcrouter/test/test_mcrouter_processing_time.py +++ b/mcrouter/test/test_mcrouter_processing_time.py @@ -10,6 +10,7 @@ import time from mcrouter.test.McrouterTestCase import McrouterTestCase +from mcrouter.test.config import McrouterGlobals from mcrouter.test.mock_servers import SleepServer @@ -35,6 +36,9 @@ def tearDown(self) -> None: os.remove(self.debug_file) def test_mcrouter_processing_time(self) -> None: + if McrouterGlobals.ossVersion(): + self.skipTest("Scuba stats are not available in the OSS build") + self.assertEqual(self.mcrouter.leaseGet("key1"), {"value": "", "token": 0}) time.sleep(0.5) self.mcrouter.terminate() diff --git a/mcrouter/test/test_mcrouter_sanity_mock.py b/mcrouter/test/test_mcrouter_sanity_mock.py index c0d81665e..b9028364a 100644 --- a/mcrouter/test/test_mcrouter_sanity_mock.py +++ b/mcrouter/test/test_mcrouter_sanity_mock.py @@ -13,6 +13,7 @@ MockMemcachedThrift, ) from mcrouter.test.McrouterTestCase import McrouterTestCase +from mcrouter.test.config import McrouterGlobals from mcrouter.test.mock_servers import DeadServer, SleepServer @@ -113,5 +114,9 @@ def make_memcached(self): class TestDualCaretSanityMock(TestMcrouterSanityMock): config = "./mcrouter/test/test_caret.json" + def setUp(self): + if McrouterGlobals.ossVersion(): + self.skipTest("The caret protocol is not available in the OSS build") + def make_memcached(self): return MockMemcachedDual(mcrouterUseThrift=False) diff --git a/mcrouter/test/test_unknown_name_handle.json b/mcrouter/test/test_unknown_name_handle.json index 7037b0b4f..798265aab 100644 --- a/mcrouter/test/test_unknown_name_handle.json +++ b/mcrouter/test/test_unknown_name_handle.json @@ -1,32 +1,32 @@ { + "pools": { + "A": { "servers": ["localhost:12345"] }, + "B": { "servers": ["localhost:12346"] } + }, "named_handles": [ { - "hash": { - "salt": "gut.texas" - }, - "name" : "B", - "pool": { - "name" : "B4", - "inherit" : "smc:twmemcache.gut.texas", - "protocol": "thrift", - "security_mech": "tls_to_plain", - "port_override": 11322, - "port_override_cross_dc": 11322, - "qos": { - "class": 2, - "path": 2 - }, - "port_override_within_dc": 11322 - }, - "type": "PoolRoute" + "type": "PoolRoute", + "name": "route:A", + "pool": "A" + }, + { + "type": "PoolRoute", + "name": "route:B", + "pool": "B" + }, + { + "type": "AllSyncRoute", + "name": "route:C", + "children": "NullRoute" + }, + { + "type": "AllSyncRoute", + "name": "route:all", + "children": ["route:A", "route:B", "route:C"] } ], - "route": { "type": "FailoverRoute", - "children": [ - "D", - "B" - ] + "children": ["D", "B"] } } diff --git a/mcrouter/test/test_warmup2.json b/mcrouter/test/test_warmup2.json index 4231a7726..79ebdbb4a 100644 --- a/mcrouter/test/test_warmup2.json +++ b/mcrouter/test/test_warmup2.json @@ -10,6 +10,7 @@ "route": { "type": "WarmUpRoute", "cold": "PoolRoute|A-cold", - "warm": "PoolRoute|A-warm" + "warm": "PoolRoute|A-warm", + "enable_metaget": true } }