From: Matthew Oliver Date: Thu, 6 Aug 2020 10:03:32 +0000 (+1000) Subject: msgr: interceptor test step enum to ease readability X-Git-Tag: v16.1.0~1301^2 X-Git-Url: http://git-server-git.apps.pok.os.sepia.ceph.com/?a=commitdiff_plain;h=2c96dab54d2ab2b6d57166415f406cbb440d59be;p=ceph.git msgr: interceptor test step enum to ease readability I've been reading messenger code and tests to help familarise myself with it. The test Interceptor is a great idea and gives breakpoint and action control for simulating different connection situations. But found the breakpoint step numbers confusing to read. These breakpoint steps do not completely correspond to steps, as they're placed in useful places for testing. This patch wraps the numbers up in an enum inside the Interceptor struct then this is used to replace the numbers to make the code more readable. I could probably extend this to using the same Interceptor::STEP enums in ProtocolV2.cc but as an initial patch I just wanted to touch the test code. Though maybe I'll do that anyway, because there's no point only making it 1/2 readable ;) The Interceptor::STEP enum names I just came up with based on where they turn up in ProtocolV2, please feel free to suggest better ones. Signed-off-by: Matthew Oliver --- diff --git a/src/msg/Messenger.h b/src/msg/Messenger.h index cd209130599..46071f0ddd4 100644 --- a/src/msg/Messenger.h +++ b/src/msg/Messenger.h @@ -59,6 +59,27 @@ struct Interceptor { STOP }; + enum STEP { + START_CLIENT_BANNER_EXCHANGE = 1, + START_SERVER_BANNER_EXCHANGE, + BANNER_EXCHANGE_BANNER_CONNECTING, + BANNER_EXCHANGE, + HANDLE_PEER_BANNER_BANNER_CONNECTING, + HANDLE_PEER_BANNER, + HANDLE_PEER_BANNER_PAYLOAD_HELLO_CONNECTING, + HANDLE_PEER_BANNER_PAYLOAD, + SEND_AUTH_REQUEST, + HANDLE_AUTH_REQUEST_ACCEPTING_SIGN, + SEND_CLIENT_IDENTITY, + SEND_SERVER_IDENTITY, + SEND_RECONNECT, + SEND_RECONNECT_OK, + READY, + HANDLE_MESSAGE, + READ_MESSAGE_COMPLETE, + SESSION_RETRY + }; + virtual ~Interceptor() {} virtual ACTION intercept(Connection *conn, uint32_t step) = 0; }; diff --git a/src/test/msgr/test_msgr.cc b/src/test/msgr/test_msgr.cc index 15f5d36f69d..4ddcafabeb5 100644 --- a/src/test/msgr/test_msgr.cc +++ b/src/test/msgr/test_msgr.cc @@ -360,9 +360,9 @@ TEST_P(MessengerTest, ConnectionRaceTest) { client_msgr->start(); // pause before sending client_ident message - cli_interceptor->breakpoint(11); + cli_interceptor->breakpoint(Interceptor::STEP::SEND_CLIENT_IDENTITY); // pause before sending client_ident message - srv_interceptor->breakpoint(11); + srv_interceptor->breakpoint(Interceptor::STEP::SEND_CLIENT_IDENTITY); ConnectionRef c2s = client_msgr->connect_to(server_msgr->get_mytype(), server_msgr->get_myaddrs()); @@ -374,17 +374,17 @@ TEST_P(MessengerTest, ConnectionRaceTest) { MPing *m2 = new MPing(); ASSERT_EQ(s2c->send_message(m2), 0); - cli_interceptor->wait(11, c2s.get()); - srv_interceptor->wait(11, s2c.get()); + cli_interceptor->wait(Interceptor::STEP::SEND_CLIENT_IDENTITY, c2s.get()); + srv_interceptor->wait(Interceptor::STEP::SEND_CLIENT_IDENTITY, s2c.get()); // at this point both connections (A->B, B->A) are paused just before sending // the client_ident message. - cli_interceptor->remove_bp(11); - srv_interceptor->remove_bp(11); + cli_interceptor->remove_bp(Interceptor::STEP::SEND_CLIENT_IDENTITY); + srv_interceptor->remove_bp(Interceptor::STEP::SEND_CLIENT_IDENTITY); - cli_interceptor->proceed(11, Interceptor::ACTION::CONTINUE); - srv_interceptor->proceed(11, Interceptor::ACTION::CONTINUE); + cli_interceptor->proceed(Interceptor::STEP::SEND_CLIENT_IDENTITY, Interceptor::ACTION::CONTINUE); + srv_interceptor->proceed(Interceptor::STEP::SEND_CLIENT_IDENTITY, Interceptor::ACTION::CONTINUE); { std::unique_lock l{cli_dispatcher.lock}; @@ -449,46 +449,46 @@ TEST_P(MessengerTest, ConnectionRaceReuseBannerTest) { client_msgr->start(); // pause before sending client_ident message - srv_interceptor->breakpoint(11); + srv_interceptor->breakpoint(Interceptor::STEP::SEND_CLIENT_IDENTITY); ConnectionRef s2c = server_msgr->connect_to(client_msgr->get_mytype(), client_msgr->get_myaddrs()); MPing *m1 = new MPing(); ASSERT_EQ(s2c->send_message(m1), 0); - srv_interceptor->wait(11); - srv_interceptor->remove_bp(11); + srv_interceptor->wait(Interceptor::STEP::SEND_CLIENT_IDENTITY); + srv_interceptor->remove_bp(Interceptor::STEP::SEND_CLIENT_IDENTITY); // pause before sending banner - cli_interceptor->breakpoint(3); + cli_interceptor->breakpoint(Interceptor::STEP::BANNER_EXCHANGE_BANNER_CONNECTING); ConnectionRef c2s = client_msgr->connect_to(server_msgr->get_mytype(), server_msgr->get_myaddrs()); MPing *m2 = new MPing(); ASSERT_EQ(c2s->send_message(m2), 0); - cli_interceptor->wait(3); - cli_interceptor->remove_bp(3); + cli_interceptor->wait(Interceptor::STEP::BANNER_EXCHANGE_BANNER_CONNECTING); + cli_interceptor->remove_bp(Interceptor::STEP::BANNER_EXCHANGE_BANNER_CONNECTING); // second connection is in BANNER_CONNECTING, ensure it stays so // and send client_ident - srv_interceptor->breakpoint(4); - srv_interceptor->proceed(11, Interceptor::ACTION::CONTINUE); + srv_interceptor->breakpoint(Interceptor::STEP::BANNER_EXCHANGE); + srv_interceptor->proceed(Interceptor::STEP::SEND_CLIENT_IDENTITY, Interceptor::ACTION::CONTINUE); // handle client_ident -- triggers reuse_connection() with exproto // in BANNER_CONNECTING - cli_interceptor->breakpoint(15); - cli_interceptor->proceed(3, Interceptor::ACTION::CONTINUE); + cli_interceptor->breakpoint(Interceptor::STEP::READY); + cli_interceptor->proceed(Interceptor::STEP::BANNER_EXCHANGE_BANNER_CONNECTING, Interceptor::ACTION::CONTINUE); - cli_interceptor->wait(15); - cli_interceptor->remove_bp(15); + cli_interceptor->wait(Interceptor::STEP::READY); + cli_interceptor->remove_bp(Interceptor::STEP::READY); // first connection is in READY - Connection *s2c_accepter = srv_interceptor->wait(4); - srv_interceptor->remove_bp(4); + Connection *s2c_accepter = srv_interceptor->wait(Interceptor::STEP::BANNER_EXCHANGE); + srv_interceptor->remove_bp(Interceptor::STEP::BANNER_EXCHANGE); - srv_interceptor->proceed(4, Interceptor::ACTION::CONTINUE); - cli_interceptor->proceed(15, Interceptor::ACTION::CONTINUE); + srv_interceptor->proceed(Interceptor::STEP::BANNER_EXCHANGE, Interceptor::ACTION::CONTINUE); + cli_interceptor->proceed(Interceptor::STEP::READY, Interceptor::ACTION::CONTINUE); { std::unique_lock l{cli_dispatcher.lock}; @@ -514,9 +514,9 @@ TEST_P(MessengerTest, ConnectionRaceReuseBannerTest) { EXPECT_FALSE(s2c_accepter->is_connected()); // established exactly once, never faulted and reconnected - EXPECT_EQ(cli_interceptor->count_step(c2s.get(), 1), 1u); - EXPECT_EQ(cli_interceptor->count_step(c2s.get(), 13), 0u); - EXPECT_EQ(cli_interceptor->count_step(c2s.get(), 15), 1u); + EXPECT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::START_CLIENT_BANNER_EXCHANGE), 1u); + EXPECT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SEND_RECONNECT), 0u); + EXPECT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::READY), 1u); client_msgr->shutdown(); client_msgr->wait(); @@ -554,23 +554,23 @@ TEST_P(MessengerTest, MissingServerIdenTest) { client_msgr->add_dispatcher_head(&cli_dispatcher); client_msgr->start(); - // pause before sending client_ident message - srv_interceptor->breakpoint(12); + // pause before sending server_ident message + srv_interceptor->breakpoint(Interceptor::STEP::SEND_SERVER_IDENTITY); ConnectionRef c2s = client_msgr->connect_to(server_msgr->get_mytype(), server_msgr->get_myaddrs()); MPing *m1 = new MPing(); ASSERT_EQ(c2s->send_message(m1), 0); - Connection *c2s_accepter = srv_interceptor->wait(12); - srv_interceptor->remove_bp(12); + Connection *c2s_accepter = srv_interceptor->wait(Interceptor::STEP::SEND_SERVER_IDENTITY); + srv_interceptor->remove_bp(Interceptor::STEP::SEND_SERVER_IDENTITY); // We inject a message from this side of the connection to force it to be // in standby when we inject the failure below MPing *m2 = new MPing(); ASSERT_EQ(c2s_accepter->send_message(m2), 0); - srv_interceptor->proceed(12, Interceptor::ACTION::FAIL); + srv_interceptor->proceed(Interceptor::STEP::SEND_SERVER_IDENTITY, Interceptor::ACTION::FAIL); { std::unique_lock l{srv_dispatcher.lock}; @@ -632,21 +632,21 @@ TEST_P(MessengerTest, MissingServerIdenTest2) { client_msgr->add_dispatcher_head(&cli_dispatcher); client_msgr->start(); - // pause before sending client_ident message - srv_interceptor->breakpoint(12); + // pause before sending server_ident message + srv_interceptor->breakpoint(Interceptor::STEP::SEND_SERVER_IDENTITY); ConnectionRef c2s = client_msgr->connect_to(server_msgr->get_mytype(), server_msgr->get_myaddrs()); - Connection *c2s_accepter = srv_interceptor->wait(12); - srv_interceptor->remove_bp(12); + Connection *c2s_accepter = srv_interceptor->wait(Interceptor::STEP::SEND_SERVER_IDENTITY); + srv_interceptor->remove_bp(Interceptor::STEP::SEND_SERVER_IDENTITY); // We inject a message from this side of the connection to force it to be // in standby when we inject the failure below MPing *m2 = new MPing(); ASSERT_EQ(c2s_accepter->send_message(m2), 0); - srv_interceptor->proceed(12, Interceptor::ACTION::FAIL); + srv_interceptor->proceed(Interceptor::STEP::SEND_SERVER_IDENTITY, Interceptor::ACTION::FAIL); { std::unique_lock l{cli_dispatcher.lock}; @@ -718,38 +718,38 @@ TEST_P(MessengerTest, ReconnectTest) { ASSERT_EQ(1u, static_cast(c2s->get_priv().get())->get_count()); ASSERT_TRUE(c2s->peer_is_osd()); - cli_interceptor->breakpoint(16); + cli_interceptor->breakpoint(Interceptor::STEP::HANDLE_MESSAGE); MPing *m2 = new MPing(); ASSERT_EQ(c2s->send_message(m2), 0); - cli_interceptor->wait(16, c2s.get()); - cli_interceptor->remove_bp(16); + cli_interceptor->wait(Interceptor::STEP::HANDLE_MESSAGE, c2s.get()); + cli_interceptor->remove_bp(Interceptor::STEP::HANDLE_MESSAGE); // at this point client and server are connected together - srv_interceptor->breakpoint(15); + srv_interceptor->breakpoint(Interceptor::STEP::READY); // failing client - cli_interceptor->proceed(16, Interceptor::ACTION::FAIL); + cli_interceptor->proceed(Interceptor::STEP::HANDLE_MESSAGE, Interceptor::ACTION::FAIL); MPing *m3 = new MPing(); ASSERT_EQ(c2s->send_message(m3), 0); - Connection *c2s_accepter = srv_interceptor->wait(15); + Connection *c2s_accepter = srv_interceptor->wait(Interceptor::STEP::READY); // the srv end of theconnection is now paused at ready // this means that the reconnect was successful - srv_interceptor->remove_bp(15); + srv_interceptor->remove_bp(Interceptor::STEP::READY); ASSERT_TRUE(c2s_accepter->peer_is_client()); // c2s_accepter sent 0 reconnect messages - ASSERT_EQ(srv_interceptor->count_step(c2s_accepter, 13), 0u); + ASSERT_EQ(srv_interceptor->count_step(c2s_accepter, Interceptor::STEP::SEND_RECONNECT), 0u); // c2s_accepter sent 1 reconnect_ok messages - ASSERT_EQ(srv_interceptor->count_step(c2s_accepter, 14), 1u); + ASSERT_EQ(srv_interceptor->count_step(c2s_accepter, Interceptor::STEP::SEND_RECONNECT_OK), 1u); // c2s sent 1 reconnect messages - ASSERT_EQ(cli_interceptor->count_step(c2s.get(), 13), 1u); + ASSERT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SEND_RECONNECT), 1u); // c2s sent 0 reconnect_ok messages - ASSERT_EQ(cli_interceptor->count_step(c2s.get(), 14), 0u); + ASSERT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SEND_RECONNECT_OK), 0u); srv_interceptor->proceed(15, Interceptor::ACTION::CONTINUE); @@ -814,43 +814,43 @@ TEST_P(MessengerTest, ReconnectRaceTest) { ASSERT_EQ(1u, static_cast(c2s->get_priv().get())->get_count()); ASSERT_TRUE(c2s->peer_is_osd()); - cli_interceptor->breakpoint(16); + cli_interceptor->breakpoint(Interceptor::STEP::HANDLE_MESSAGE); MPing *m2 = new MPing(); ASSERT_EQ(c2s->send_message(m2), 0); - cli_interceptor->wait(16, c2s.get()); - cli_interceptor->remove_bp(16); + cli_interceptor->wait(Interceptor::STEP::HANDLE_MESSAGE, c2s.get()); + cli_interceptor->remove_bp(Interceptor::STEP::HANDLE_MESSAGE); // at this point client and server are connected together // force both client and server to race on reconnect - cli_interceptor->breakpoint(13); - srv_interceptor->breakpoint(13); + cli_interceptor->breakpoint(Interceptor::STEP::SEND_RECONNECT); + srv_interceptor->breakpoint(Interceptor::STEP::SEND_RECONNECT); // failing client // this will cause both client and server to reconnect at the same time - cli_interceptor->proceed(16, Interceptor::ACTION::FAIL); + cli_interceptor->proceed(Interceptor::STEP::HANDLE_MESSAGE, Interceptor::ACTION::FAIL); MPing *m3 = new MPing(); ASSERT_EQ(c2s->send_message(m3), 0); - cli_interceptor->wait(13, c2s.get()); - srv_interceptor->wait(13); + cli_interceptor->wait(Interceptor::STEP::SEND_RECONNECT, c2s.get()); + srv_interceptor->wait(Interceptor::STEP::SEND_RECONNECT); - cli_interceptor->remove_bp(13); - srv_interceptor->remove_bp(13); + cli_interceptor->remove_bp(Interceptor::STEP::SEND_RECONNECT); + srv_interceptor->remove_bp(Interceptor::STEP::SEND_RECONNECT); // pause on "ready" - srv_interceptor->breakpoint(15); + srv_interceptor->breakpoint(Interceptor::STEP::READY); - cli_interceptor->proceed(13, Interceptor::ACTION::CONTINUE); - srv_interceptor->proceed(13, Interceptor::ACTION::CONTINUE); + cli_interceptor->proceed(Interceptor::STEP::SEND_RECONNECT, Interceptor::ACTION::CONTINUE); + srv_interceptor->proceed(Interceptor::STEP::SEND_RECONNECT, Interceptor::ACTION::CONTINUE); - Connection *c2s_accepter = srv_interceptor->wait(15); + Connection *c2s_accepter = srv_interceptor->wait(Interceptor::STEP::READY); // the server has reconnected and is "ready" - srv_interceptor->remove_bp(15); + srv_interceptor->remove_bp(Interceptor::STEP::READY); ASSERT_TRUE(c2s_accepter->peer_is_client()); ASSERT_TRUE(c2s->peer_is_osd()); @@ -858,24 +858,24 @@ TEST_P(MessengerTest, ReconnectRaceTest) { // the server should win the reconnect race // c2s_accepter sent 1 or 2 reconnect messages - ASSERT_LT(srv_interceptor->count_step(c2s_accepter, 13), 3u); - ASSERT_GT(srv_interceptor->count_step(c2s_accepter, 13), 0u); + ASSERT_LT(srv_interceptor->count_step(c2s_accepter, Interceptor::STEP::SEND_RECONNECT), 3u); + ASSERT_GT(srv_interceptor->count_step(c2s_accepter, Interceptor::STEP::SEND_RECONNECT), 0u); // c2s_accepter sent 0 reconnect_ok messages - ASSERT_EQ(srv_interceptor->count_step(c2s_accepter, 14), 0u); + ASSERT_EQ(srv_interceptor->count_step(c2s_accepter, Interceptor::STEP::SEND_RECONNECT_OK), 0u); // c2s sent 1 reconnect messages - ASSERT_EQ(cli_interceptor->count_step(c2s.get(), 13), 1u); + ASSERT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SEND_RECONNECT), 1u); // c2s sent 1 reconnect_ok messages - ASSERT_EQ(cli_interceptor->count_step(c2s.get(), 14), 1u); + ASSERT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SEND_RECONNECT_OK), 1u); - if (srv_interceptor->count_step(c2s_accepter, 13) == 2) { + if (srv_interceptor->count_step(c2s_accepter, Interceptor::STEP::SEND_RECONNECT) == 2) { // if the server send the reconnect message two times then // the client must have sent a session retry message to the server - ASSERT_EQ(cli_interceptor->count_step(c2s.get(), 18), 1u); + ASSERT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SESSION_RETRY), 1u); } else { - ASSERT_EQ(cli_interceptor->count_step(c2s.get(), 18), 0u); + ASSERT_EQ(cli_interceptor->count_step(c2s.get(), Interceptor::STEP::SESSION_RETRY), 0u); } - srv_interceptor->proceed(15, Interceptor::ACTION::CONTINUE); + srv_interceptor->proceed(Interceptor::STEP::READY, Interceptor::ACTION::CONTINUE); { std::unique_lock l{cli_dispatcher.lock};