From ae8770a3176c439bbd30efb316439154d558851c Mon Sep 17 00:00:00 2001 From: yang-g Date: Mon, 11 May 2020 11:57:19 -0700 Subject: [PATCH 1/3] Let the connectivity test use local subchannel pool to create multiple connections --- .../surface/sequential_connectivity_test.cc | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/test/core/surface/sequential_connectivity_test.cc b/test/core/surface/sequential_connectivity_test.cc index ed20017d708..9af60204a12 100644 --- a/test/core/surface/sequential_connectivity_test.cc +++ b/test/core/surface/sequential_connectivity_test.cc @@ -18,6 +18,7 @@ #include #include +#include #include #include @@ -133,7 +134,18 @@ static void insecure_test_add_port(grpc_server* server, const char* addr) { } static grpc_channel* insecure_test_create_channel(const char* addr) { - return grpc_insecure_channel_create(addr, nullptr, nullptr); + grpc_arg arg = {GRPC_ARG_INTEGER, + const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), + {.integer = 1}}; + grpc_channel_args* new_client_args = + grpc_channel_args_copy_and_add(nullptr, &arg, 1); + grpc_channel* channel = + grpc_insecure_channel_create(addr, new_client_args, nullptr); + { + grpc_core::ExecCtx exec_ctx; + grpc_channel_args_destroy(new_client_args); + } + return channel; } static const test_fixture insecure_test = { @@ -170,12 +182,16 @@ static grpc_channel* secure_test_create_channel(const char* addr) { grpc_channel_credentials* ssl_creds = grpc_ssl_credentials_create(test_root_cert, nullptr, nullptr, nullptr); grpc_slice_unref(ca_slice); - grpc_arg ssl_name_override = { - GRPC_ARG_STRING, - const_cast(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG), - {const_cast("foo.test.google.fr")}}; + const int kNumArgs = 2; + grpc_arg args[kNumArgs] = { + {GRPC_ARG_STRING, + const_cast(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG), + {const_cast("foo.test.google.fr")}}, + {GRPC_ARG_INTEGER, + const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), + {.integer = 1}}}; grpc_channel_args* new_client_args = - grpc_channel_args_copy_and_add(nullptr, &ssl_name_override, 1); + grpc_channel_args_copy_and_add(nullptr, args, kNumArgs); grpc_channel* channel = grpc_secure_channel_create(ssl_creds, addr, new_client_args, nullptr); { From 52b3c66e536cb3044145c377ba445f7b446c5e8c Mon Sep 17 00:00:00 2001 From: yang-g Date: Mon, 11 May 2020 14:52:36 -0700 Subject: [PATCH 2/3] Cover both subchannel sharing/non-sharing cases in test --- .../surface/sequential_connectivity_test.cc | 124 +++++++++--------- 1 file changed, 64 insertions(+), 60 deletions(-) diff --git a/test/core/surface/sequential_connectivity_test.cc b/test/core/surface/sequential_connectivity_test.cc index 9af60204a12..a838237aef5 100644 --- a/test/core/surface/sequential_connectivity_test.cc +++ b/test/core/surface/sequential_connectivity_test.cc @@ -37,16 +37,15 @@ typedef struct test_fixture { const char* name; void (*add_server_port)(grpc_server* server, const char* addr); - grpc_channel* (*create_channel)(const char* addr); + grpc_channel* (*create_channel)(const char* addr, + grpc_channel_credentials* creds, + bool share_subchannel); + // Have the creds here so all the channels will share the same one to enabled + // subchannel sharing if needed. + grpc_channel_credentials* creds; } test_fixture; -/* TODO(yashykt): When our macos testing infrastructure becomes good enough, we - * wouldn't need to reduce the number of connections on MacOS */ -#ifdef __APPLE__ #define NUM_CONNECTIONS 100 -#else -#define NUM_CONNECTIONS 1000 -#endif /* __APPLE__ */ typedef struct { grpc_server* server; @@ -62,8 +61,32 @@ static void server_thread_func(void* args) { GPR_ASSERT(ev.success == true); } -static void run_test(const test_fixture* fixture) { - gpr_log(GPR_INFO, "TEST: %s", fixture->name); +static grpc_channel* create_test_channel(const char* addr, + grpc_channel_credentials* creds, + bool share_subchannel) { + grpc_channel* channel = nullptr; + const int kMaxArgs = 2; + grpc_arg args[kMaxArgs]; + size_t arg_count = 0; + args[arg_count++] = grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), !share_subchannel); + if (creds != nullptr) { + args[arg_count++] = grpc_channel_arg_string_create( + const_cast(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG), + const_cast("foo.test.google.fr")); + } + grpc_channel_args channel_args = {arg_count, args}; + if (creds != nullptr) { + channel = grpc_secure_channel_create(creds, addr, &channel_args, nullptr); + } else { + channel = grpc_insecure_channel_create(addr, &channel_args, nullptr); + } + return channel; +} + +static void run_test(const test_fixture* fixture, bool share_subchannel) { + gpr_log(GPR_INFO, "TEST: %s sharing subchannel: %d", fixture->name, + share_subchannel); grpc_init(); @@ -84,7 +107,8 @@ static void run_test(const test_fixture* fixture) { grpc_completion_queue* cq = grpc_completion_queue_create_for_next(nullptr); grpc_channel* channels[NUM_CONNECTIONS]; for (size_t i = 0; i < NUM_CONNECTIONS; i++) { - channels[i] = fixture->create_channel(addr.c_str()); + channels[i] = + fixture->create_channel(addr.c_str(), fixture->creds, share_subchannel); gpr_timespec connect_deadline = grpc_timeout_seconds_to_deadline(30); grpc_connectivity_state state; @@ -133,27 +157,11 @@ static void insecure_test_add_port(grpc_server* server, const char* addr) { grpc_server_add_insecure_http2_port(server, addr); } -static grpc_channel* insecure_test_create_channel(const char* addr) { - grpc_arg arg = {GRPC_ARG_INTEGER, - const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), - {.integer = 1}}; - grpc_channel_args* new_client_args = - grpc_channel_args_copy_and_add(nullptr, &arg, 1); - grpc_channel* channel = - grpc_insecure_channel_create(addr, new_client_args, nullptr); - { - grpc_core::ExecCtx exec_ctx; - grpc_channel_args_destroy(new_client_args); - } - return channel; +static grpc_channel* insecure_test_create_channel( + const char* addr, grpc_channel_credentials* creds, bool share_subchannel) { + return create_test_channel(addr, creds, share_subchannel); } -static const test_fixture insecure_test = { - "insecure", - insecure_test_add_port, - insecure_test_create_channel, -}; - static void secure_test_add_port(grpc_server* server, const char* addr) { grpc_slice cert_slice, key_slice; GPR_ASSERT(GRPC_LOG_IF_ERROR( @@ -173,7 +181,25 @@ static void secure_test_add_port(grpc_server* server, const char* addr) { grpc_server_credentials_release(ssl_creds); } -static grpc_channel* secure_test_create_channel(const char* addr) { +static grpc_channel* secure_test_create_channel(const char* addr, + grpc_channel_credentials* creds, + bool share_subchannel) { + return create_test_channel(addr, creds, share_subchannel); +} + +int main(int argc, char** argv) { + grpc::testing::TestEnvironment env(argc, argv); + + const test_fixture insecure_test = { + "insecure", + insecure_test_add_port, + insecure_test_create_channel, + nullptr, + }; + + run_test(&insecure_test, /*share_subchannel=*/true); + run_test(&insecure_test, /*share_subchannel=*/false); + grpc_slice ca_slice; GPR_ASSERT(GRPC_LOG_IF_ERROR("load_file", grpc_load_file(CA_CERT_PATH, 1, &ca_slice))); @@ -182,35 +208,13 @@ static grpc_channel* secure_test_create_channel(const char* addr) { grpc_channel_credentials* ssl_creds = grpc_ssl_credentials_create(test_root_cert, nullptr, nullptr, nullptr); grpc_slice_unref(ca_slice); - const int kNumArgs = 2; - grpc_arg args[kNumArgs] = { - {GRPC_ARG_STRING, - const_cast(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG), - {const_cast("foo.test.google.fr")}}, - {GRPC_ARG_INTEGER, - const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), - {.integer = 1}}}; - grpc_channel_args* new_client_args = - grpc_channel_args_copy_and_add(nullptr, args, kNumArgs); - grpc_channel* channel = - grpc_secure_channel_create(ssl_creds, addr, new_client_args, nullptr); - { - grpc_core::ExecCtx exec_ctx; - grpc_channel_args_destroy(new_client_args); - } + const test_fixture secure_test = { + "secure", + secure_test_add_port, + secure_test_create_channel, + ssl_creds, + }; + run_test(&secure_test, /*share_subchannel=*/true); + run_test(&secure_test, /*share_subchannel=*/false); grpc_channel_credentials_release(ssl_creds); - return channel; -} - -static const test_fixture secure_test = { - "secure", - secure_test_add_port, - secure_test_create_channel, -}; - -int main(int argc, char** argv) { - grpc::testing::TestEnvironment env(argc, argv); - - run_test(&insecure_test); - run_test(&secure_test); } From 9c63d75e20a74d354ced3138d579e4c953dd532c Mon Sep 17 00:00:00 2001 From: yang-g Date: Mon, 11 May 2020 15:42:37 -0700 Subject: [PATCH 3/3] Resolve comments --- .../surface/sequential_connectivity_test.cc | 35 ++++++------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/test/core/surface/sequential_connectivity_test.cc b/test/core/surface/sequential_connectivity_test.cc index a838237aef5..d0ed9a76ec0 100644 --- a/test/core/surface/sequential_connectivity_test.cc +++ b/test/core/surface/sequential_connectivity_test.cc @@ -16,6 +16,8 @@ * */ +#include + #include #include #include @@ -37,9 +39,6 @@ typedef struct test_fixture { const char* name; void (*add_server_port)(grpc_server* server, const char* addr); - grpc_channel* (*create_channel)(const char* addr, - grpc_channel_credentials* creds, - bool share_subchannel); // Have the creds here so all the channels will share the same one to enabled // subchannel sharing if needed. grpc_channel_credentials* creds; @@ -65,17 +64,16 @@ static grpc_channel* create_test_channel(const char* addr, grpc_channel_credentials* creds, bool share_subchannel) { grpc_channel* channel = nullptr; - const int kMaxArgs = 2; - grpc_arg args[kMaxArgs]; - size_t arg_count = 0; - args[arg_count++] = grpc_channel_arg_integer_create( - const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), !share_subchannel); + std::vector args; + args.push_back(grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), + !share_subchannel)); if (creds != nullptr) { - args[arg_count++] = grpc_channel_arg_string_create( + args.push_back(grpc_channel_arg_string_create( const_cast(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG), - const_cast("foo.test.google.fr")); + const_cast("foo.test.google.fr"))); } - grpc_channel_args channel_args = {arg_count, args}; + grpc_channel_args channel_args = {args.size(), args.data()}; if (creds != nullptr) { channel = grpc_secure_channel_create(creds, addr, &channel_args, nullptr); } else { @@ -108,7 +106,7 @@ static void run_test(const test_fixture* fixture, bool share_subchannel) { grpc_channel* channels[NUM_CONNECTIONS]; for (size_t i = 0; i < NUM_CONNECTIONS; i++) { channels[i] = - fixture->create_channel(addr.c_str(), fixture->creds, share_subchannel); + create_test_channel(addr.c_str(), fixture->creds, share_subchannel); gpr_timespec connect_deadline = grpc_timeout_seconds_to_deadline(30); grpc_connectivity_state state; @@ -157,11 +155,6 @@ static void insecure_test_add_port(grpc_server* server, const char* addr) { grpc_server_add_insecure_http2_port(server, addr); } -static grpc_channel* insecure_test_create_channel( - const char* addr, grpc_channel_credentials* creds, bool share_subchannel) { - return create_test_channel(addr, creds, share_subchannel); -} - static void secure_test_add_port(grpc_server* server, const char* addr) { grpc_slice cert_slice, key_slice; GPR_ASSERT(GRPC_LOG_IF_ERROR( @@ -181,19 +174,12 @@ static void secure_test_add_port(grpc_server* server, const char* addr) { grpc_server_credentials_release(ssl_creds); } -static grpc_channel* secure_test_create_channel(const char* addr, - grpc_channel_credentials* creds, - bool share_subchannel) { - return create_test_channel(addr, creds, share_subchannel); -} - int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); const test_fixture insecure_test = { "insecure", insecure_test_add_port, - insecure_test_create_channel, nullptr, }; @@ -211,7 +197,6 @@ int main(int argc, char** argv) { const test_fixture secure_test = { "secure", secure_test_add_port, - secure_test_create_channel, ssl_creds, }; run_test(&secure_test, /*share_subchannel=*/true);