From 0baf1dc569a112bc71039b84bdaef0bad68cf852 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 28 Oct 2016 04:44:01 +0200 Subject: [PATCH 1/8] Made LB token dynamic size <= 50 bytes --- src/core/ext/lb_policy/grpclb/grpclb.c | 8 +++++--- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h | 4 ++-- src/proto/grpc/lb/v1/load_balancer.options | 2 +- test/cpp/grpclb/grpclb_test.cc | 2 -- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 6da4febf266..5bbf857079f 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -407,10 +407,12 @@ static grpc_lb_addresses *process_serverlist( /* lb token processing */ void *user_data; if (server->has_load_balance_token) { - const size_t lb_token_size = - GPR_ARRAY_SIZE(server->load_balance_token) - 1; + const size_t lb_token_max_length = + GPR_ARRAY_SIZE(server->load_balance_token); + const size_t lb_token_length = + strnlen(server->load_balance_token, lb_token_max_length); grpc_mdstr *lb_token_mdstr = grpc_mdstr_from_buffer( - (uint8_t *)server->load_balance_token, lb_token_size); + (uint8_t *)server->load_balance_token, lb_token_length); user_data = grpc_mdelem_from_metadata_strings(GRPC_MDSTR_LB_TOKEN, lb_token_mdstr); } else { diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h index 53fed22bae0..e36d0966f85 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h @@ -77,7 +77,7 @@ typedef struct _grpc_lb_v1_Server { bool has_port; int32_t port; bool has_load_balance_token; - char load_balance_token[65]; + char load_balance_token[50]; bool has_drop_request; bool drop_request; /* @@protoc_insertion_point(struct:grpc_lb_v1_Server) */ @@ -172,7 +172,7 @@ extern const pb_field_t grpc_lb_v1_Server_fields[5]; #define grpc_lb_v1_LoadBalanceResponse_size (98 + grpc_lb_v1_ServerList_size) #define grpc_lb_v1_InitialLoadBalanceResponse_size 90 /* grpc_lb_v1_ServerList_size depends on runtime parameters */ -#define grpc_lb_v1_Server_size 98 +#define grpc_lb_v1_Server_size 83 /* Message IDs (where set with "msgid" option) */ #ifdef PB_MSGID diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index a9398d5f474..f4afc1ee61d 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -2,5 +2,5 @@ grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 grpc.lb.v1.InitialLoadBalanceResponse.client_config max_size:64 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 -grpc.lb.v1.Server.load_balance_token max_size:65 +grpc.lb.v1.Server.load_balance_token max_size:50 load_balancer.proto no_unions:true diff --git a/test/cpp/grpclb/grpclb_test.cc b/test/cpp/grpclb/grpclb_test.cc index b6056f9ae4c..d7882404381 100644 --- a/test/cpp/grpclb/grpclb_test.cc +++ b/test/cpp/grpclb/grpclb_test.cc @@ -144,7 +144,6 @@ static gpr_slice build_response_payload_slice( // disfunctional implementation of std::to_string in gcc 4.4, which doesn't // have a version for int but does have one for long long int. string token_data = "token" + std::to_string((long long int)ports[i]); - token_data.resize(64, '-'); server->set_load_balance_token(token_data); } const grpc::string &enc_resp = response.SerializeAsString(); @@ -333,7 +332,6 @@ static void start_backend_server(server_fixture *sf) { // disfunctional implementation of std::to_string in gcc 4.4, which doesn't // have a version for int but does have one for long long int. string expected_token = "token" + std::to_string((long long int)sf->port); - expected_token.resize(64, '-'); GPR_ASSERT(contains_metadata(&request_metadata_recv, "lb-token", expected_token.c_str())); From 472fc2ffd10026ff6f61886c5a8c26863aae966b Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 28 Oct 2016 21:01:26 +0200 Subject: [PATCH 2/8] Updated proto field comment with max token length --- src/proto/grpc/lb/v1/load_balancer.proto | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/proto/grpc/lb/v1/load_balancer.proto b/src/proto/grpc/lb/v1/load_balancer.proto index 210fba1323e..b6849c10d7d 100644 --- a/src/proto/grpc/lb/v1/load_balancer.proto +++ b/src/proto/grpc/lb/v1/load_balancer.proto @@ -130,6 +130,8 @@ message Server { // frontend requests for that pick must include the token in its initial // metadata. The token is used by the backend to verify the request and to // allow the backend to report load to the gRPC LB system. + // + // Its length is variable but less than 50 bytes. string load_balance_token = 3; // Indicates whether this particular request should be dropped by the client From 2a23c60a4b7734623f3c1ae49478d182a25b6421 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sat, 29 Oct 2016 01:26:17 +0200 Subject: [PATCH 3/8] Removed spurious option for lb nanopb proto --- src/proto/grpc/lb/v1/load_balancer.options | 1 - 1 file changed, 1 deletion(-) diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index f4afc1ee61d..7fbd44b9ded 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -1,5 +1,4 @@ grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 -grpc.lb.v1.InitialLoadBalanceResponse.client_config max_size:64 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 grpc.lb.v1.Server.load_balance_token max_size:50 From 41752d6e7bb3b414139e854678bae2a962f0d82e Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sat, 29 Oct 2016 01:26:30 +0200 Subject: [PATCH 4/8] Added max length comments for some lb proto fields --- src/proto/grpc/lb/v1/load_balancer.proto | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/proto/grpc/lb/v1/load_balancer.proto b/src/proto/grpc/lb/v1/load_balancer.proto index b6849c10d7d..7736a14b9ae 100644 --- a/src/proto/grpc/lb/v1/load_balancer.proto +++ b/src/proto/grpc/lb/v1/load_balancer.proto @@ -63,7 +63,8 @@ message LoadBalanceRequest { } message InitialLoadBalanceRequest { - // Name of load balanced service (IE, service.grpc.gslb.google.com) + // Name of load balanced service (IE, service.grpc.gslb.google.com). Its + // length should be less than 128 bytes. string name = 1; } @@ -95,7 +96,8 @@ message InitialLoadBalanceResponse { // This is an application layer redirect that indicates the client should use // the specified server for load balancing. When this field is non-empty in // the response, the client should open a separate connection to the - // load_balancer_delegate and call the BalanceLoad method. + // load_balancer_delegate and call the BalanceLoad method. Its length should + // be less than 64 bytes. string load_balancer_delegate = 1; // This interval defines how often the client should send the client stats From f37cd4f84448d9a9bb96cb16e1f616811e42f9af Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sun, 30 Oct 2016 15:18:27 +0100 Subject: [PATCH 5/8] Updated max length of load balanced service to 255 bytes --- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c | 9 +-------- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h | 6 +++--- src/proto/grpc/lb/v1/load_balancer.options | 2 +- src/proto/grpc/lb/v1/load_balancer.proto | 2 +- 4 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c index afecb716fb0..c90d3f9ff59 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c @@ -106,14 +106,7 @@ PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) #endif #if !defined(PB_FIELD_16BIT) && !defined(PB_FIELD_32BIT) -/* If you get an error here, it means that you need to define PB_FIELD_16BIT - * compile-time option. You can do that in pb.h or on compiler command line. - * - * The reason you need to do this is that some of your messages contain tag - * numbers or field sizes that are larger than what can fit in the default - * 8 bit descriptors. - */ -PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceRequest, client_stats) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, initial_response) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, server_list) < 256 && pb_membersize(grpc_lb_v1_InitialLoadBalanceResponse, client_stats_report_interval) < 256 && pb_membersize(grpc_lb_v1_ServerList, servers) < 256 && pb_membersize(grpc_lb_v1_ServerList, expiration_interval) < 256), YOU_MUST_DEFINE_PB_FIELD_16BIT_FOR_MESSAGES_grpc_lb_v1_Duration_grpc_lb_v1_LoadBalanceRequest_grpc_lb_v1_InitialLoadBalanceRequest_grpc_lb_v1_ClientStats_grpc_lb_v1_LoadBalanceResponse_grpc_lb_v1_InitialLoadBalanceResponse_grpc_lb_v1_ServerList_grpc_lb_v1_Server) +#error Field descriptor for grpc_lb_v1_InitialLoadBalanceRequest.name is too large. Define PB_FIELD_16BIT to fix this. #endif diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h index e36d0966f85..81e89254d17 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h @@ -66,7 +66,7 @@ typedef struct _grpc_lb_v1_Duration { typedef struct _grpc_lb_v1_InitialLoadBalanceRequest { bool has_name; - char name[128]; + char name[256]; /* @@protoc_insertion_point(struct:grpc_lb_v1_InitialLoadBalanceRequest) */ } grpc_lb_v1_InitialLoadBalanceRequest; @@ -166,8 +166,8 @@ extern const pb_field_t grpc_lb_v1_Server_fields[5]; /* Maximum encoded size of messages (where known) */ #define grpc_lb_v1_Duration_size 22 -#define grpc_lb_v1_LoadBalanceRequest_size 169 -#define grpc_lb_v1_InitialLoadBalanceRequest_size 131 +#define grpc_lb_v1_LoadBalanceRequest_size 297 +#define grpc_lb_v1_InitialLoadBalanceRequest_size 259 #define grpc_lb_v1_ClientStats_size 33 #define grpc_lb_v1_LoadBalanceResponse_size (98 + grpc_lb_v1_ServerList_size) #define grpc_lb_v1_InitialLoadBalanceResponse_size 90 diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index 7fbd44b9ded..46e27fa2c11 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -1,4 +1,4 @@ -grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 +grpc.lb.v1.InitialLoadBalanceRequest.name max_size:256 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 grpc.lb.v1.Server.load_balance_token max_size:50 diff --git a/src/proto/grpc/lb/v1/load_balancer.proto b/src/proto/grpc/lb/v1/load_balancer.proto index 7736a14b9ae..44a5150a7e0 100644 --- a/src/proto/grpc/lb/v1/load_balancer.proto +++ b/src/proto/grpc/lb/v1/load_balancer.proto @@ -64,7 +64,7 @@ message LoadBalanceRequest { message InitialLoadBalanceRequest { // Name of load balanced service (IE, service.grpc.gslb.google.com). Its - // length should be less than 128 bytes. + // length should be less than 256 bytes. string name = 1; } From 5a4005772d4d5025c6ffc704dcfec7fcc6cafe97 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Mon, 31 Oct 2016 17:20:15 -0700 Subject: [PATCH 6/8] Don't use the stack so much Because internally we like to keep our thread stacks tiny. --- .../end2end/tests/resource_quota_server.c | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/test/core/end2end/tests/resource_quota_server.c b/test/core/end2end/tests/resource_quota_server.c index fbd4986dfba..47c5ed6abb7 100644 --- a/test/core/end2end/tests/resource_quota_server.c +++ b/test/core/end2end/tests/resource_quota_server.c @@ -137,17 +137,22 @@ void resource_quota_server(grpc_end2end_test_config config) { * will be verified on completion. */ gpr_slice request_payload_slice = generate_random_slice(); - grpc_call *client_calls[NUM_CALLS]; - grpc_call *server_calls[NUM_CALLS]; - grpc_metadata_array initial_metadata_recv[NUM_CALLS]; - grpc_metadata_array trailing_metadata_recv[NUM_CALLS]; - grpc_metadata_array request_metadata_recv[NUM_CALLS]; - grpc_call_details call_details[NUM_CALLS]; - grpc_status_code status[NUM_CALLS]; - char *details[NUM_CALLS]; - size_t details_capacity[NUM_CALLS]; - grpc_byte_buffer *request_payload_recv[NUM_CALLS]; - int was_cancelled[NUM_CALLS]; + grpc_call **client_calls = malloc(sizeof(grpc_call *) * NUM_CALLS); + grpc_call **server_calls = malloc(sizeof(grpc_call *) * NUM_CALLS); + grpc_metadata_array *initial_metadata_recv = + malloc(sizeof(grpc_metadata_array) * NUM_CALLS); + grpc_metadata_array *trailing_metadata_recv = + malloc(sizeof(grpc_metadata_array) * NUM_CALLS); + grpc_metadata_array *request_metadata_recv = + malloc(sizeof(grpc_metadata_array) * NUM_CALLS); + grpc_call_details *call_details = + malloc(sizeof(grpc_call_details) * NUM_CALLS); + grpc_status_code *status = malloc(sizeof(grpc_status_code) * NUM_CALLS); + char **details = malloc(sizeof(char *) * NUM_CALLS); + size_t *details_capacity = malloc(sizeof(size_t) * NUM_CALLS); + grpc_byte_buffer **request_payload_recv = + malloc(sizeof(grpc_byte_buffer *) * NUM_CALLS); + int *was_cancelled = malloc(sizeof(int) * NUM_CALLS); grpc_call_error error; int pending_client_calls = 0; int pending_server_start_calls = 0; @@ -356,6 +361,18 @@ void resource_quota_server(grpc_end2end_test_config config) { gpr_slice_unref(request_payload_slice); grpc_resource_quota_unref(resource_quota); + free(client_calls); + free(server_calls); + free(initial_metadata_recv); + free(trailing_metadata_recv); + free(request_metadata_recv); + free(call_details); + free(status); + free(details); + free(details_capacity); + free(request_payload_recv); + free(was_cancelled); + end_test(&f); config.tear_down_data(&f); } From d14458f5894444aeaf3793d29d3c3d299493ecd1 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 13:19:40 -0700 Subject: [PATCH 7/8] Reverted back to using 128 bytes for service name --- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c | 9 ++++++++- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h | 6 +++--- src/proto/grpc/lb/v1/load_balancer.options | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c index c90d3f9ff59..afecb716fb0 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c @@ -106,7 +106,14 @@ PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) #endif #if !defined(PB_FIELD_16BIT) && !defined(PB_FIELD_32BIT) -#error Field descriptor for grpc_lb_v1_InitialLoadBalanceRequest.name is too large. Define PB_FIELD_16BIT to fix this. +/* If you get an error here, it means that you need to define PB_FIELD_16BIT + * compile-time option. You can do that in pb.h or on compiler command line. + * + * The reason you need to do this is that some of your messages contain tag + * numbers or field sizes that are larger than what can fit in the default + * 8 bit descriptors. + */ +PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceRequest, client_stats) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, initial_response) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, server_list) < 256 && pb_membersize(grpc_lb_v1_InitialLoadBalanceResponse, client_stats_report_interval) < 256 && pb_membersize(grpc_lb_v1_ServerList, servers) < 256 && pb_membersize(grpc_lb_v1_ServerList, expiration_interval) < 256), YOU_MUST_DEFINE_PB_FIELD_16BIT_FOR_MESSAGES_grpc_lb_v1_Duration_grpc_lb_v1_LoadBalanceRequest_grpc_lb_v1_InitialLoadBalanceRequest_grpc_lb_v1_ClientStats_grpc_lb_v1_LoadBalanceResponse_grpc_lb_v1_InitialLoadBalanceResponse_grpc_lb_v1_ServerList_grpc_lb_v1_Server) #endif diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h index 81e89254d17..e36d0966f85 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h @@ -66,7 +66,7 @@ typedef struct _grpc_lb_v1_Duration { typedef struct _grpc_lb_v1_InitialLoadBalanceRequest { bool has_name; - char name[256]; + char name[128]; /* @@protoc_insertion_point(struct:grpc_lb_v1_InitialLoadBalanceRequest) */ } grpc_lb_v1_InitialLoadBalanceRequest; @@ -166,8 +166,8 @@ extern const pb_field_t grpc_lb_v1_Server_fields[5]; /* Maximum encoded size of messages (where known) */ #define grpc_lb_v1_Duration_size 22 -#define grpc_lb_v1_LoadBalanceRequest_size 297 -#define grpc_lb_v1_InitialLoadBalanceRequest_size 259 +#define grpc_lb_v1_LoadBalanceRequest_size 169 +#define grpc_lb_v1_InitialLoadBalanceRequest_size 131 #define grpc_lb_v1_ClientStats_size 33 #define grpc_lb_v1_LoadBalanceResponse_size (98 + grpc_lb_v1_ServerList_size) #define grpc_lb_v1_InitialLoadBalanceResponse_size 90 diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index 46e27fa2c11..7fbd44b9ded 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -1,4 +1,4 @@ -grpc.lb.v1.InitialLoadBalanceRequest.name max_size:256 +grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 grpc.lb.v1.Server.load_balance_token max_size:50 From c939022dd5b1496c77d6e69a9677b141fc1378fa Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 1 Nov 2016 15:47:24 -0700 Subject: [PATCH 8/8] More data massaging --- tools/run_tests/performance/bq_upload_result.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/run_tests/performance/bq_upload_result.py b/tools/run_tests/performance/bq_upload_result.py index 3a3c4ae7003..0ea23d22122 100755 --- a/tools/run_tests/performance/bq_upload_result.py +++ b/tools/run_tests/performance/bq_upload_result.py @@ -122,6 +122,8 @@ def _flatten_result_inplace(scenario_result): scenario_result['clientSuccess'] = json.dumps(scenario_result['clientSuccess']) scenario_result['serverSuccess'] = json.dumps(scenario_result['serverSuccess']) scenario_result['requestResults'] = json.dumps(scenario_result.get('requestResults', [])) + scenario_result['summary'].pop('successfulRequestsPerSecond', None) + scenario_result['summary'].pop('failedRequestsPerSecond', None) def _populate_metadata_inplace(scenario_result):