From a9cf87d9e456621297afccdcfc9072df7630840c Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 7 Nov 2016 17:43:15 -0800 Subject: [PATCH 1/6] Fixed wrong connectivity status updates for RR --- .../ext/lb_policy/round_robin/round_robin.c | 310 +++++++++++------- 1 file changed, 193 insertions(+), 117 deletions(-) diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 0fd3abe0992..f83ae1b1e94 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -59,6 +59,7 @@ * the subchannel by the caller. */ +#include #include #include @@ -116,8 +117,13 @@ typedef struct { grpc_closure connectivity_changed_closure; /** this subchannels current position in subchannel->ready_list */ ready_list *ready_list_node; - /** last observed connectivity */ - grpc_connectivity_state connectivity_state; + /** last observed connectivity. Not updated by + * \a grpc_subchannel_notify_on_state_change. Used to determine the previous + * state while processing the new state in \a rr_connectivity_changed */ + grpc_connectivity_state prev_connectivity_state; + /** current connectivity state. Updated by \a + * grpc_subchannel_notify_on_state_change */ + grpc_connectivity_state curr_connectivity_state; /** the subchannel's target user data */ void *user_data; /** vtable to operate over \a user_data */ @@ -127,6 +133,7 @@ typedef struct { struct round_robin_lb_policy { /** base policy: must be first */ grpc_lb_policy base; + gpr_mu mu; /** total number of addresses received at creation time */ size_t num_addresses; @@ -135,8 +142,11 @@ struct round_robin_lb_policy { size_t num_subchannels; subchannel_data **subchannels; - /** mutex protecting remaining members */ - gpr_mu mu; + /** how many subchannels are in TRANSIENT_FAILURE */ + size_t num_transient_failures; + /** how many subchannels are IDLE */ + size_t num_idle; + /** have we started picking? */ int started_picking; /** are we shutting down? */ @@ -258,6 +268,10 @@ static void remove_disconnected_sc_locked(round_robin_lb_policy *p, gpr_free(node); } +static bool is_ready_list_empty(round_robin_lb_policy *p) { + return p->ready_list.prev == NULL; +} + static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { round_robin_lb_policy *p = (round_robin_lb_policy *)pol; ready_list *elem; @@ -268,7 +282,7 @@ static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { for (size_t i = 0; i < p->num_subchannels; i++) { subchannel_data *sd = p->subchannels[i]; - GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin_destroy"); + GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "rr_destroy"); if (sd->user_data != NULL) { GPR_ASSERT(sd->user_data_vtable != NULL); sd->user_data_vtable->destroy(sd->user_data); @@ -381,18 +395,18 @@ static void start_picking(grpc_exec_ctx *exec_ctx, round_robin_lb_policy *p) { size_t i; p->started_picking = 1; - if (grpc_lb_round_robin_trace) { - gpr_log(GPR_DEBUG, "LB_POLICY: p=%p num_subchannels=%" PRIuPTR, (void *)p, - p->num_subchannels); - } - for (i = 0; i < p->num_subchannels; i++) { subchannel_data *sd = p->subchannels[i]; - sd->connectivity_state = GRPC_CHANNEL_IDLE; + /* use some sentinel value outside of the range of grpc_connectivity_state + * to signal an undefined previous state. We won't be referring to this + * value again and it'll be overwritten after the first call to + * rr_connectivity_changed */ + sd->prev_connectivity_state = INT_MAX; + sd->curr_connectivity_state = GRPC_CHANNEL_IDLE; + GRPC_LB_POLICY_WEAK_REF(&p->base, "rr_connectivity"); grpc_subchannel_notify_on_state_change( exec_ctx, sd->subchannel, p->base.interested_parties, - &sd->connectivity_state, &sd->connectivity_changed_closure); - GRPC_LB_POLICY_WEAK_REF(&p->base, "round_robin_connectivity"); + &sd->curr_connectivity_state, &sd->connectivity_changed_closure); } } @@ -453,125 +467,182 @@ static int rr_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, } } +static void update_state_counters(subchannel_data *sd) { + round_robin_lb_policy *p = sd->policy; + + /* update p->num_transient_failures (resp. p->num_idle): if the previous + * state was TRANSIENT_FAILURE (resp. IDLE), decrement + * p->num_transient_failures (resp. p->num_idle). */ + if (sd->prev_connectivity_state == GRPC_CHANNEL_TRANSIENT_FAILURE) { + GPR_ASSERT(p->num_transient_failures > 0); + --p->num_transient_failures; + } else if (sd->prev_connectivity_state == GRPC_CHANNEL_IDLE) { + GPR_ASSERT(p->num_idle > 0); + --p->num_idle; + } +} + +/* sd is the subchannel_data associted with the updated subchannel. + * shutdown_error will only be used upon policy transition to TRANSIENT_FAILURE + * or SHUTDOWN */ +static grpc_connectivity_state update_lb_connectivity_status( + grpc_exec_ctx *exec_ctx, subchannel_data *sd, grpc_error *error) { + /* In priority order. The first rule to match terminates the search (ie, if we + * are on rule n, all previous rules were unfulfilled). + * + * 1) RULE: ANY subchannel is READY => policy is READY. + * CHECK: At least one subchannel is ready iff p->ready_list is NOT empty. + * + * 2) RULE: ANY subchannel is CONNECTING => policy is CONNECTING. + * CHECK: sd->curr_connectivity_state == CONNECTING. + * + * 3) RULE: ALL subchannels are SHUTDOWN => policy is SHUTDOWN. + * CHECK: p->num_subchannels = 0. + * + * 4) RULE: ALL subchannels are TRANSIENT_FAILURE => policy is + * TRANSIENT_FAILURE. + * CHECK: p->num_transient_failures == p->num_subchannels. + * + * 5) RULE: ALL subchannels are IDLE => policy is IDLE. + * CHECK: p->num_idle == p->num_subchannels. + */ + round_robin_lb_policy *p = sd->policy; + if (!is_ready_list_empty(p)) { /* 1) READY */ + grpc_connectivity_state_set(exec_ctx, &p->state_tracker, GRPC_CHANNEL_READY, + GRPC_ERROR_NONE, "rr_ready"); + return GRPC_CHANNEL_READY; + } else if (sd->curr_connectivity_state == + GRPC_CHANNEL_CONNECTING) { /* 2) CONNECTING */ + grpc_connectivity_state_set(exec_ctx, &p->state_tracker, + GRPC_CHANNEL_CONNECTING, GRPC_ERROR_NONE, + "rr_connecting"); + return GRPC_CHANNEL_CONNECTING; + } else if (p->num_subchannels == 0) { /* 3) SHUTDOWN */ + grpc_connectivity_state_set(exec_ctx, &p->state_tracker, + GRPC_CHANNEL_SHUTDOWN, GRPC_ERROR_REF(error), + "rr_shutdown"); + return GRPC_CHANNEL_SHUTDOWN; + } else if (p->num_transient_failures == + p->num_subchannels) { /* 4) TRANSIENT_FAILURE */ + grpc_connectivity_state_set(exec_ctx, &p->state_tracker, + GRPC_CHANNEL_TRANSIENT_FAILURE, + GRPC_ERROR_REF(error), "rr_transient_failure"); + return GRPC_CHANNEL_TRANSIENT_FAILURE; + } else if (p->num_idle == p->num_subchannels) { /* 5) IDLE */ + grpc_connectivity_state_set(exec_ctx, &p->state_tracker, GRPC_CHANNEL_IDLE, + GRPC_ERROR_NONE, "rr_idle"); + return GRPC_CHANNEL_IDLE; + } + /* no change */ + return sd->curr_connectivity_state; +} + static void rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { subchannel_data *sd = arg; round_robin_lb_policy *p = sd->policy; pending_pick *pp; - int unref = 0; - GRPC_ERROR_REF(error); gpr_mu_lock(&p->mu); if (p->shutdown) { - unref = 1; - } else { - switch (sd->connectivity_state) { - case GRPC_CHANNEL_READY: - grpc_connectivity_state_set(exec_ctx, &p->state_tracker, - GRPC_CHANNEL_READY, GRPC_ERROR_REF(error), - "connecting_ready"); - /* add the newly connected subchannel to the list of connected ones. - * Note that it goes to the "end of the line". */ - sd->ready_list_node = add_connected_sc_locked(p, sd); - /* at this point we know there's at least one suitable subchannel. Go - * ahead and pick one and notify the pending suitors in - * p->pending_picks. This preemtively replicates rr_pick()'s actions. */ - ready_list *selected = peek_next_connected_locked(p); - GPR_ASSERT(selected != NULL); - if (p->pending_picks != NULL) { - /* if the selected subchannel is going to be used for the pending - * picks, update the last picked pointer */ - advance_last_picked_locked(p); + gpr_mu_unlock(&p->mu); + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &p->base, "rr_connectivity"); + GRPC_ERROR_UNREF(error); + return; + } + switch (sd->curr_connectivity_state) { + case GRPC_CHANNEL_READY: + /* add the newly connected subchannel to the list of connected ones. + * Note that it goes to the "end of the line". */ + sd->ready_list_node = add_connected_sc_locked(p, sd); + /* at this point we know there's at least one suitable subchannel. Go + * ahead and pick one and notify the pending suitors in + * p->pending_picks. This preemtively replicates rr_pick()'s actions. */ + ready_list *selected = peek_next_connected_locked(p); + GPR_ASSERT(selected != NULL); + if (p->pending_picks != NULL) { + /* if the selected subchannel is going to be used for the pending + * picks, update the last picked pointer */ + advance_last_picked_locked(p); + } + while ((pp = p->pending_picks)) { + p->pending_picks = pp->next; + *pp->target = GRPC_CONNECTED_SUBCHANNEL_REF( + grpc_subchannel_get_connected_subchannel(selected->subchannel), + "rr_picked"); + if (pp->user_data != NULL) { + *pp->user_data = selected->user_data; } - + if (grpc_lb_round_robin_trace) { + gpr_log(GPR_DEBUG, + "[RR CONN CHANGED] TARGET <-- SUBCHANNEL %p (NODE %p)", + (void *)selected->subchannel, (void *)selected); + } + grpc_exec_ctx_sched(exec_ctx, pp->on_complete, GRPC_ERROR_NONE, NULL); + gpr_free(pp); + } + /* renew notification: reuses the "rr_connectivity" weak ref */ + grpc_subchannel_notify_on_state_change( + exec_ctx, sd->subchannel, p->base.interested_parties, + &sd->curr_connectivity_state, &sd->connectivity_changed_closure); + update_lb_connectivity_status(exec_ctx, sd, error); + sd->prev_connectivity_state = sd->curr_connectivity_state; + break; + case GRPC_CHANNEL_IDLE: + ++p->num_idle; + /* fallthrough */ + case GRPC_CHANNEL_CONNECTING: + update_state_counters(sd); + /* renew notification: reuses the "rr_connectivity" weak ref */ + grpc_subchannel_notify_on_state_change( + exec_ctx, sd->subchannel, p->base.interested_parties, + &sd->curr_connectivity_state, &sd->connectivity_changed_closure); + update_lb_connectivity_status(exec_ctx, sd, error); + sd->prev_connectivity_state = sd->curr_connectivity_state; + break; + case GRPC_CHANNEL_TRANSIENT_FAILURE: + ++p->num_transient_failures; + /* remove from ready list if still present */ + if (sd->ready_list_node != NULL) { + remove_disconnected_sc_locked(p, sd->ready_list_node); + sd->ready_list_node = NULL; + } + /* renew notification: reuses the "rr_connectivity" weak ref */ + grpc_subchannel_notify_on_state_change( + exec_ctx, sd->subchannel, p->base.interested_parties, + &sd->curr_connectivity_state, &sd->connectivity_changed_closure); + update_lb_connectivity_status(exec_ctx, sd, error); + sd->prev_connectivity_state = sd->curr_connectivity_state; + break; + case GRPC_CHANNEL_SHUTDOWN: + update_state_counters(sd); + if (sd->ready_list_node != NULL) { + remove_disconnected_sc_locked(p, sd->ready_list_node); + sd->ready_list_node = NULL; + } + --p->num_subchannels; + GPR_SWAP(subchannel_data *, p->subchannels[sd->index], + p->subchannels[p->num_subchannels]); + GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "rr_subchannel_shutdown"); + p->subchannels[sd->index]->index = sd->index; + if (update_lb_connectivity_status(exec_ctx, sd, error) == + GRPC_CHANNEL_SHUTDOWN) { + /* the policy is shutting down. Flush all the pending picks... */ while ((pp = p->pending_picks)) { p->pending_picks = pp->next; - - *pp->target = GRPC_CONNECTED_SUBCHANNEL_REF( - grpc_subchannel_get_connected_subchannel(selected->subchannel), - "picked"); - if (pp->user_data != NULL) { - *pp->user_data = selected->user_data; - } - if (grpc_lb_round_robin_trace) { - gpr_log(GPR_DEBUG, - "[RR CONN CHANGED] TARGET <-- SUBCHANNEL %p (NODE %p)", - (void *)selected->subchannel, (void *)selected); - } + *pp->target = NULL; grpc_exec_ctx_sched(exec_ctx, pp->on_complete, GRPC_ERROR_NONE, NULL); gpr_free(pp); } - grpc_subchannel_notify_on_state_change( - exec_ctx, sd->subchannel, p->base.interested_parties, - &sd->connectivity_state, &sd->connectivity_changed_closure); - break; - case GRPC_CHANNEL_CONNECTING: - case GRPC_CHANNEL_IDLE: - grpc_connectivity_state_set( - exec_ctx, &p->state_tracker, sd->connectivity_state, - GRPC_ERROR_REF(error), "connecting_changed"); - grpc_subchannel_notify_on_state_change( - exec_ctx, sd->subchannel, p->base.interested_parties, - &sd->connectivity_state, &sd->connectivity_changed_closure); - break; - case GRPC_CHANNEL_TRANSIENT_FAILURE: - /* renew state notification */ - grpc_subchannel_notify_on_state_change( - exec_ctx, sd->subchannel, p->base.interested_parties, - &sd->connectivity_state, &sd->connectivity_changed_closure); - - /* remove from ready list if still present */ - if (sd->ready_list_node != NULL) { - remove_disconnected_sc_locked(p, sd->ready_list_node); - sd->ready_list_node = NULL; - } - grpc_connectivity_state_set( - exec_ctx, &p->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, - GRPC_ERROR_REF(error), "connecting_transient_failure"); - break; - case GRPC_CHANNEL_SHUTDOWN: - if (sd->ready_list_node != NULL) { - remove_disconnected_sc_locked(p, sd->ready_list_node); - sd->ready_list_node = NULL; - } - - p->num_subchannels--; - GPR_SWAP(subchannel_data *, p->subchannels[sd->index], - p->subchannels[p->num_subchannels]); - GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin"); - p->subchannels[sd->index]->index = sd->index; - gpr_free(sd); - - unref = 1; - if (p->num_subchannels == 0) { - grpc_connectivity_state_set( - exec_ctx, &p->state_tracker, GRPC_CHANNEL_SHUTDOWN, - GRPC_ERROR_CREATE_REFERENCING("Round Robin Channels Exhausted", - &error, 1), - "no_more_channels"); - while ((pp = p->pending_picks)) { - p->pending_picks = pp->next; - *pp->target = NULL; - grpc_exec_ctx_sched(exec_ctx, pp->on_complete, GRPC_ERROR_NONE, - NULL); - gpr_free(pp); - } - } else { - grpc_connectivity_state_set( - exec_ctx, &p->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, - GRPC_ERROR_REF(error), "subchannel_failed"); - } - } /* switch */ - } /* !unref */ - - gpr_mu_unlock(&p->mu); - - if (unref) { - GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &p->base, "round_robin_connectivity"); + } + gpr_free(sd); + /* unref the "rr_connectivity" weak ref from start_picking */ + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &p->base, "rr_connectivity"); + break; } - + gpr_mu_unlock(&p->mu); GRPC_ERROR_UNREF(error); } @@ -607,9 +678,9 @@ static void rr_ping_one(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, gpr_mu_unlock(&p->mu); target = GRPC_CONNECTED_SUBCHANNEL_REF( grpc_subchannel_get_connected_subchannel(selected->subchannel), - "picked"); + "rr_picked"); grpc_connected_subchannel_ping(exec_ctx, target, closure); - GRPC_CONNECTED_SUBCHANNEL_UNREF(exec_ctx, target, "picked"); + GRPC_CONNECTED_SUBCHANNEL_UNREF(exec_ctx, target, "rr_picked"); } else { gpr_mu_unlock(&p->mu); grpc_exec_ctx_sched(exec_ctx, closure, @@ -705,6 +776,11 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx, grpc_lb_policy_init(&p->base, &round_robin_lb_policy_vtable); grpc_connectivity_state_init(&p->state_tracker, GRPC_CHANNEL_IDLE, "round_robin"); + + if (grpc_lb_round_robin_trace) { + gpr_log(GPR_DEBUG, "Created RR policy at %p with %lu subchannels", + (void *)p, (unsigned long)p->num_subchannels); + } gpr_mu_init(&p->mu); return &p->base; } From ea6689dbdfcb5ff9b599e0e54afa8c7ea40e3718 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 8 Nov 2016 09:46:41 -0800 Subject: [PATCH 2/6] Added INIT state to grpc_connectivity_state --- include/grpc/impl/codegen/connectivity_state.h | 2 ++ src/core/ext/lb_policy/pick_first/pick_first.c | 2 ++ src/core/ext/lb_policy/round_robin/round_robin.c | 5 +++-- src/core/lib/transport/connectivity_state.c | 3 +++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/include/grpc/impl/codegen/connectivity_state.h b/include/grpc/impl/codegen/connectivity_state.h index 71865d8a566..c6d1a334053 100644 --- a/include/grpc/impl/codegen/connectivity_state.h +++ b/include/grpc/impl/codegen/connectivity_state.h @@ -40,6 +40,8 @@ extern "C" { /** Connectivity state of a channel. */ typedef enum { + /** channel has just been initialized */ + GRPC_CHANNEL_INIT = -1, /** channel is idle */ GRPC_CHANNEL_IDLE, /** channel is connecting */ diff --git a/src/core/ext/lb_policy/pick_first/pick_first.c b/src/core/ext/lb_policy/pick_first/pick_first.c index ac3c6a305ac..39f3b3d60f8 100644 --- a/src/core/ext/lb_policy/pick_first/pick_first.c +++ b/src/core/ext/lb_policy/pick_first/pick_first.c @@ -292,6 +292,8 @@ static void pf_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, } else { loop: switch (p->checking_connectivity) { + case GRPC_CHANNEL_INIT: + GPR_UNREACHABLE_CODE(); case GRPC_CHANNEL_READY: grpc_connectivity_state_set(exec_ctx, &p->state_tracker, GRPC_CHANNEL_READY, GRPC_ERROR_NONE, diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index f83ae1b1e94..a07aef3815c 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -59,7 +59,6 @@ * the subchannel by the caller. */ -#include #include #include @@ -401,7 +400,7 @@ static void start_picking(grpc_exec_ctx *exec_ctx, round_robin_lb_policy *p) { * to signal an undefined previous state. We won't be referring to this * value again and it'll be overwritten after the first call to * rr_connectivity_changed */ - sd->prev_connectivity_state = INT_MAX; + sd->prev_connectivity_state = GRPC_CHANNEL_INIT; sd->curr_connectivity_state = GRPC_CHANNEL_IDLE; GRPC_LB_POLICY_WEAK_REF(&p->base, "rr_connectivity"); grpc_subchannel_notify_on_state_change( @@ -553,6 +552,8 @@ static void rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, return; } switch (sd->curr_connectivity_state) { + case GRPC_CHANNEL_INIT: + GPR_UNREACHABLE_CODE(); case GRPC_CHANNEL_READY: /* add the newly connected subchannel to the list of connected ones. * Note that it goes to the "end of the line". */ diff --git a/src/core/lib/transport/connectivity_state.c b/src/core/lib/transport/connectivity_state.c index fdb53078143..89072879d94 100644 --- a/src/core/lib/transport/connectivity_state.c +++ b/src/core/lib/transport/connectivity_state.c @@ -43,6 +43,8 @@ int grpc_connectivity_state_trace = 0; const char *grpc_connectivity_state_name(grpc_connectivity_state state) { switch (state) { + case GRPC_CHANNEL_INIT: + return "INIT"; case GRPC_CHANNEL_IDLE: return "IDLE"; case GRPC_CHANNEL_CONNECTING: @@ -159,6 +161,7 @@ void grpc_connectivity_state_set(grpc_exec_ctx *exec_ctx, grpc_error_free_string(error_string); } switch (state) { + case GRPC_CHANNEL_INIT: case GRPC_CHANNEL_CONNECTING: case GRPC_CHANNEL_IDLE: case GRPC_CHANNEL_READY: From e9448df2fc106df555513217ba267d6381559ffa Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Wed, 9 Nov 2016 15:55:17 -0800 Subject: [PATCH 3/6] Fixed wrong ordering of operations --- src/core/ext/lb_policy/round_robin/round_robin.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index a07aef3815c..acb2f77e456 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -435,7 +435,7 @@ static int rr_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, /* readily available, report right away */ *target = GRPC_CONNECTED_SUBCHANNEL_REF( grpc_subchannel_get_connected_subchannel(selected->subchannel), - "picked"); + "rr_picked"); if (user_data != NULL) { *user_data = selected->user_data; @@ -584,24 +584,24 @@ static void rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_exec_ctx_sched(exec_ctx, pp->on_complete, GRPC_ERROR_NONE, NULL); gpr_free(pp); } + update_lb_connectivity_status(exec_ctx, sd, error); + sd->prev_connectivity_state = sd->curr_connectivity_state; /* renew notification: reuses the "rr_connectivity" weak ref */ grpc_subchannel_notify_on_state_change( exec_ctx, sd->subchannel, p->base.interested_parties, &sd->curr_connectivity_state, &sd->connectivity_changed_closure); - update_lb_connectivity_status(exec_ctx, sd, error); - sd->prev_connectivity_state = sd->curr_connectivity_state; break; case GRPC_CHANNEL_IDLE: ++p->num_idle; /* fallthrough */ case GRPC_CHANNEL_CONNECTING: update_state_counters(sd); + update_lb_connectivity_status(exec_ctx, sd, error); + sd->prev_connectivity_state = sd->curr_connectivity_state; /* renew notification: reuses the "rr_connectivity" weak ref */ grpc_subchannel_notify_on_state_change( exec_ctx, sd->subchannel, p->base.interested_parties, &sd->curr_connectivity_state, &sd->connectivity_changed_closure); - update_lb_connectivity_status(exec_ctx, sd, error); - sd->prev_connectivity_state = sd->curr_connectivity_state; break; case GRPC_CHANNEL_TRANSIENT_FAILURE: ++p->num_transient_failures; @@ -610,12 +610,12 @@ static void rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, remove_disconnected_sc_locked(p, sd->ready_list_node); sd->ready_list_node = NULL; } + update_lb_connectivity_status(exec_ctx, sd, error); + sd->prev_connectivity_state = sd->curr_connectivity_state; /* renew notification: reuses the "rr_connectivity" weak ref */ grpc_subchannel_notify_on_state_change( exec_ctx, sd->subchannel, p->base.interested_parties, &sd->curr_connectivity_state, &sd->connectivity_changed_closure); - update_lb_connectivity_status(exec_ctx, sd, error); - sd->prev_connectivity_state = sd->curr_connectivity_state; break; case GRPC_CHANNEL_SHUTDOWN: update_state_counters(sd); From 5f50a1baaa92eb6b361a8e5f4cf0b3b31623380d Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 14 Nov 2016 17:43:15 -0800 Subject: [PATCH 4/6] Update test_get_channel_info() to use new ch arg syntax --- test/core/client_channel/lb_policies_test.c | 29 +++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c index 6e317eb9a94..7b1c47e9dda 100644 --- a/test/core/client_channel/lb_policies_test.c +++ b/test/core/client_channel/lb_policies_test.c @@ -641,6 +641,34 @@ static void test_pending_calls(size_t concurrent_calls) { test_spec_destroy(spec); } +static void test_get_channel_info() { + grpc_channel_args args; + grpc_arg arg_array[1]; + arg_array[0].type = GRPC_ARG_STRING; + arg_array[0].key = GRPC_ARG_LB_POLICY_NAME; + arg_array[0].value.string = "round_robin"; + args.num_args = 1; + args.args = arg_array; + + grpc_channel *channel = + grpc_insecure_channel_create("ipv4:127.0.0.1:1234", &args, NULL); + // Ensures that resolver returns. + grpc_channel_check_connectivity_state(channel, true /* try_to_connect */); + // Use grpc_channel_get_info() to get LB policy name. + char *lb_policy_name = NULL; + grpc_channel_info channel_info; + channel_info.lb_policy_name = &lb_policy_name; + grpc_channel_get_info(channel, &channel_info); + GPR_ASSERT(lb_policy_name != NULL); + GPR_ASSERT(strcmp(lb_policy_name, "round_robin") == 0); + gpr_free(lb_policy_name); + // Try again without requesting anything. This is a no-op. + channel_info.lb_policy_name = NULL; + grpc_channel_get_info(channel, &channel_info); + // Clean up. + grpc_channel_destroy(channel); +} + static void print_failed_expectations(const int *expected_connection_sequence, const int *actual_connection_sequence, const size_t expected_seq_length, @@ -935,6 +963,7 @@ int main(int argc, char **argv) { test_pending_calls(4); test_ping(); + test_get_channel_info(); grpc_exec_ctx_finish(&exec_ctx); grpc_shutdown(); From 01dbb902743adfaa6c07792faa139fed0bc9c891 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 14 Nov 2016 22:32:29 -0800 Subject: [PATCH 5/6] Added testing for connectivity states --- test/core/client_channel/lb_policies_test.c | 324 ++++++++++++-------- 1 file changed, 197 insertions(+), 127 deletions(-) diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c index 7b1c47e9dda..f4779d39351 100644 --- a/test/core/client_channel/lb_policies_test.c +++ b/test/core/client_channel/lb_policies_test.c @@ -62,8 +62,14 @@ typedef struct servers_fixture { grpc_metadata_array *request_metadata_recv; } servers_fixture; +typedef struct request_sequences { + size_t n; + int *connections; + int *connectivity_states; +} request_sequences; + typedef void (*verifier_fn)(const servers_fixture *, grpc_channel *, - const int *, const size_t); + const request_sequences *, const size_t); typedef struct test_spec { size_t num_iters; @@ -227,9 +233,24 @@ static void teardown_servers(servers_fixture *f) { gpr_free(f); } +static request_sequences request_sequences_create(size_t n) { + request_sequences res; + res.n = n; + res.connections = gpr_malloc(sizeof(*res.connections) * n); + res.connectivity_states = gpr_malloc(sizeof(*res.connectivity_states) * n); + return res; +} + +static void request_sequences_destroy(const request_sequences *rseqs) { + gpr_free(rseqs->connections); + gpr_free(rseqs->connectivity_states); +} + /** Returns connection sequence (server indices), which must be freed */ -static int *perform_request(servers_fixture *f, grpc_channel *client, - request_data *rdata, const test_spec *spec) { +static request_sequences perform_request(servers_fixture *f, + grpc_channel *client, + request_data *rdata, + const test_spec *spec) { grpc_call *c; int s_idx; int *s_valid; @@ -239,11 +260,10 @@ static int *perform_request(servers_fixture *f, grpc_channel *client, size_t i, iter_num; grpc_event ev; int read_tag; - int *connection_sequence; int completed_client; + const request_sequences sequences = request_sequences_create(spec->num_iters); s_valid = gpr_malloc(sizeof(int) * f->num_servers); - connection_sequence = gpr_malloc(sizeof(int) * spec->num_iters); for (iter_num = 0; iter_num < spec->num_iters; iter_num++) { cq_verifier *cqv = cq_verifier_create(f->cq); @@ -260,7 +280,7 @@ static int *perform_request(servers_fixture *f, grpc_channel *client, } } - connection_sequence[iter_num] = -1; + sequences.connections[iter_num] = -1; grpc_metadata_array_init(&rdata->initial_metadata_recv); grpc_metadata_array_init(&rdata->trailing_metadata_recv); @@ -305,12 +325,14 @@ static int *perform_request(servers_fixture *f, grpc_channel *client, grpc_call_start_batch(c, ops, (size_t)(op - ops), tag(1), NULL)); s_idx = -1; - while ( - (ev = grpc_completion_queue_next( - f->cq, GRPC_TIMEOUT_MILLIS_TO_DEADLINE(10 * RETRY_TIMEOUT), NULL)) - .type != GRPC_QUEUE_TIMEOUT) { + while ((ev = grpc_completion_queue_next( + f->cq, GRPC_TIMEOUT_MILLIS_TO_DEADLINE(RETRY_TIMEOUT), NULL)) + .type != GRPC_QUEUE_TIMEOUT) { GPR_ASSERT(ev.type == GRPC_OP_COMPLETE); read_tag = ((int)(intptr_t)ev.tag); + const grpc_connectivity_state conn_state = + grpc_channel_check_connectivity_state(client, 0); + sequences.connectivity_states[iter_num] = conn_state; gpr_log(GPR_DEBUG, "EVENT: success:%d, type:%d, tag:%d iter:%" PRIuPTR, ev.success, ev.type, read_tag, iter_num); if (ev.success && read_tag >= 1000) { @@ -318,7 +340,7 @@ static int *perform_request(servers_fixture *f, grpc_channel *client, /* only server notifications for non-shutdown events */ s_idx = read_tag - 1000; s_valid[s_idx] = 1; - connection_sequence[iter_num] = s_idx; + sequences.connections[iter_num] = s_idx; break; } else if (read_tag == 1) { gpr_log(GPR_DEBUG, "client timed out"); @@ -381,10 +403,9 @@ static int *perform_request(servers_fixture *f, grpc_channel *client, } } - GPR_ASSERT( - grpc_completion_queue_next( - f->cq, GRPC_TIMEOUT_MILLIS_TO_DEADLINE(2 * RETRY_TIMEOUT), NULL) - .type == GRPC_QUEUE_TIMEOUT); + GPR_ASSERT(grpc_completion_queue_next( + f->cq, GRPC_TIMEOUT_MILLIS_TO_DEADLINE(RETRY_TIMEOUT), NULL) + .type == GRPC_QUEUE_TIMEOUT); grpc_metadata_array_destroy(&rdata->initial_metadata_recv); grpc_metadata_array_destroy(&rdata->trailing_metadata_recv); @@ -401,7 +422,7 @@ static int *perform_request(servers_fixture *f, grpc_channel *client, gpr_free(s_valid); - return connection_sequence; + return sequences; } static grpc_call **perform_multirequest(servers_fixture *f, @@ -441,62 +462,10 @@ static grpc_call **perform_multirequest(servers_fixture *f, return calls; } -static void assert_channel_connectivity(grpc_channel *ch, - size_t num_accepted_conn_states, - int accepted_conn_state, ...) { - size_t i; - grpc_channel_stack *client_stack; - grpc_channel_element *client_channel_filter; - grpc_connectivity_state actual_conn_state; - grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - va_list ap; - - client_stack = grpc_channel_get_channel_stack(ch); - client_channel_filter = grpc_channel_stack_last_element(client_stack); - - actual_conn_state = grpc_client_channel_check_connectivity_state( - &exec_ctx, client_channel_filter, 0 /* don't try to connect */); - grpc_exec_ctx_finish(&exec_ctx); - va_start(ap, accepted_conn_state); - for (i = 0; i < num_accepted_conn_states; i++) { - if ((int)actual_conn_state == accepted_conn_state) { - break; - } - accepted_conn_state = va_arg(ap, grpc_connectivity_state); - } - va_end(ap); - if (i == num_accepted_conn_states) { - char **accepted_strs = - gpr_malloc(sizeof(char *) * num_accepted_conn_states); - char *accepted_str_joined; - va_start(ap, accepted_conn_state); - for (i = 0; i < num_accepted_conn_states; i++) { - GPR_ASSERT(gpr_asprintf(&accepted_strs[i], "%d", accepted_conn_state) > - 0); - accepted_conn_state = va_arg(ap, grpc_connectivity_state); - } - va_end(ap); - accepted_str_joined = gpr_strjoin_sep((const char **)accepted_strs, - num_accepted_conn_states, ", ", NULL); - gpr_log( - GPR_ERROR, - "Channel connectivity assertion failed: expected , got %d", - accepted_str_joined, actual_conn_state); - - for (i = 0; i < num_accepted_conn_states; i++) { - gpr_free(accepted_strs[i]); - } - gpr_free(accepted_strs); - gpr_free(accepted_str_joined); - abort(); - } -} - void run_spec(const test_spec *spec) { grpc_channel *client; char *client_hostport; char *servers_hostports_str; - int *actual_connection_sequence; request_data rdata; servers_fixture *f; grpc_channel_args args; @@ -524,14 +493,14 @@ void run_spec(const test_spec *spec) { gpr_log(GPR_INFO, "Testing '%s' with servers=%s client=%s", spec->description, servers_hostports_str, client_hostport); - actual_connection_sequence = perform_request(f, client, &rdata, spec); + const request_sequences sequences = perform_request(f, client, &rdata, spec); - spec->verifier(f, client, actual_connection_sequence, spec->num_iters); + spec->verifier(f, client, &sequences, spec->num_iters); gpr_free(client_hostport); gpr_free(servers_hostports_str); - gpr_free(actual_connection_sequence); gpr_free(rdata.call_details); + request_sequences_destroy(&sequences); grpc_channel_destroy(client); /* calls the LB's shutdown func */ teardown_servers(f); @@ -684,29 +653,43 @@ static void print_failed_expectations(const int *expected_connection_sequence, static void verify_vanilla_round_robin(const servers_fixture *f, grpc_channel *client, - const int *actual_connection_sequence, + const request_sequences *sequences, const size_t num_iters) { - int *expected_connection_sequence; - size_t i; const size_t expected_seq_length = f->num_servers; /* verify conn. seq. expectation */ /* get the first sequence of "num_servers" elements */ - expected_connection_sequence = gpr_malloc(sizeof(int) * expected_seq_length); - memcpy(expected_connection_sequence, actual_connection_sequence, + int *expected_connection_sequence = + gpr_malloc(sizeof(int) * expected_seq_length); + memcpy(expected_connection_sequence, sequences->connections, sizeof(int) * expected_seq_length); - for (i = 0; i < num_iters; i++) { - const int actual = actual_connection_sequence[i]; + for (size_t i = 0; i < num_iters; i++) { + const int actual = sequences->connections[i]; const int expected = expected_connection_sequence[i % expected_seq_length]; if (actual != expected) { - print_failed_expectations(expected_connection_sequence, - actual_connection_sequence, expected_seq_length, - num_iters); + gpr_log( + GPR_ERROR, + "CONNECTION SEQUENCE FAILURE: expected %d, got %d at iteration #%d", + expected, actual, (int)i); + abort(); + } + } + + /* All servers are available, therefore all client subchannels are READY, even + * when we only need one for the client channel state to be READY */ + for (size_t i = 0; i < sequences->n; i++) { + const grpc_connectivity_state actual = sequences->connectivity_states[i]; + const grpc_connectivity_state expected = GRPC_CHANNEL_READY; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), (int)i); abort(); } } - assert_channel_connectivity(client, 1, GRPC_CHANNEL_READY); gpr_free(expected_connection_sequence); } @@ -715,7 +698,7 @@ static void verify_vanilla_round_robin(const servers_fixture *f, * given in "f") are killed */ static void verify_vanishing_floor_round_robin( const servers_fixture *f, grpc_channel *client, - const int *actual_connection_sequence, const size_t num_iters) { + const request_sequences *sequences, const size_t num_iters) { int *expected_connection_sequence; const size_t expected_seq_length = 2; size_t i; @@ -723,57 +706,83 @@ static void verify_vanishing_floor_round_robin( /* verify conn. seq. expectation */ /* copy the first full sequence (without -1s) */ expected_connection_sequence = gpr_malloc(sizeof(int) * expected_seq_length); - memcpy(expected_connection_sequence, actual_connection_sequence + 2, + memcpy(expected_connection_sequence, sequences->connections + 2, expected_seq_length * sizeof(int)); /* first two elements of the sequence should be [0 (1st server), -1 (failure)] */ - GPR_ASSERT(actual_connection_sequence[0] == 0); - GPR_ASSERT(actual_connection_sequence[1] == -1); + GPR_ASSERT(sequences->connections[0] == 0); + GPR_ASSERT(sequences->connections[1] == -1); /* the next two element must be [3, 0], repeating from that point: the 3 is * brought forth by servers 1 and 2 disappearing after the intial pick of 0 */ - GPR_ASSERT(actual_connection_sequence[2] == 3); - GPR_ASSERT(actual_connection_sequence[3] == 0); + GPR_ASSERT(sequences->connections[2] == 3); + GPR_ASSERT(sequences->connections[3] == 0); /* make sure that the expectation obliges */ for (i = 2; i < num_iters; i++) { - const int actual = actual_connection_sequence[i]; + const int actual = sequences->connections[i]; const int expected = expected_connection_sequence[i % expected_seq_length]; if (actual != expected) { print_failed_expectations(expected_connection_sequence, - actual_connection_sequence, expected_seq_length, + sequences->connections, expected_seq_length, num_iters); abort(); } } + + /* There's always at least one subchannel READY (connected), therefore the + * overall state of the client channel is READY at all times. */ + for (i = 0; i < sequences->n; i++) { + const grpc_connectivity_state actual = sequences->connectivity_states[i]; + const grpc_connectivity_state expected = GRPC_CHANNEL_READY; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), (int)i); + abort(); + } + } + gpr_free(expected_connection_sequence); } -static void verify_total_carnage_round_robin( - const servers_fixture *f, grpc_channel *client, - const int *actual_connection_sequence, const size_t num_iters) { - size_t i; - - for (i = 0; i < num_iters; i++) { - const int actual = actual_connection_sequence[i]; +static void verify_total_carnage_round_robin(const servers_fixture *f, + grpc_channel *client, + const request_sequences *sequences, + const size_t num_iters) { + for (size_t i = 0; i < num_iters; i++) { + const int actual = sequences->connections[i]; const int expected = -1; if (actual != expected) { - gpr_log(GPR_ERROR, "FAILURE: expected %d, actual %d at iter %" PRIuPTR, - expected, actual, i); + gpr_log( + GPR_ERROR, + "CONNECTION SEQUENCE FAILURE: expected %d, got %d at iteration #%d", + expected, actual, (int)i); abort(); } } - /* even though we know all the servers are dead, the client is still trying - * retrying, believing it's in a transient failure situation */ - assert_channel_connectivity(client, 2, GRPC_CHANNEL_TRANSIENT_FAILURE, - GRPC_CHANNEL_CONNECTING); + /* no server is ever available. The persistent state is TRANSIENT_FAILURE */ + for (size_t i = 0; i < sequences->n; i++) { + const grpc_connectivity_state actual = sequences->connectivity_states[i]; + const grpc_connectivity_state expected = GRPC_CHANNEL_TRANSIENT_FAILURE; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), (int)i); + abort(); + } + } } static void verify_partial_carnage_round_robin( const servers_fixture *f, grpc_channel *client, - const int *actual_connection_sequence, const size_t num_iters) { + const request_sequences *sequences, const size_t num_iters) { int *expected_connection_sequence; size_t i; const size_t expected_seq_length = f->num_servers; @@ -781,15 +790,15 @@ static void verify_partial_carnage_round_robin( /* verify conn. seq. expectation */ /* get the first sequence of "num_servers" elements */ expected_connection_sequence = gpr_malloc(sizeof(int) * expected_seq_length); - memcpy(expected_connection_sequence, actual_connection_sequence, + memcpy(expected_connection_sequence, sequences->connections, sizeof(int) * expected_seq_length); for (i = 0; i < num_iters / 2; i++) { - const int actual = actual_connection_sequence[i]; + const int actual = sequences->connections[i]; const int expected = expected_connection_sequence[i % expected_seq_length]; if (actual != expected) { print_failed_expectations(expected_connection_sequence, - actual_connection_sequence, expected_seq_length, + sequences->connections, expected_seq_length, num_iters); abort(); } @@ -797,13 +806,34 @@ static void verify_partial_carnage_round_robin( /* second half of the iterations go without response */ for (; i < num_iters; i++) { - GPR_ASSERT(actual_connection_sequence[i] == -1); + GPR_ASSERT(sequences->connections[i] == -1); + } + + /* We can assert that the first client channel state should be READY, when all + * servers were available; and that the last one should be TRANSIENT_FAILURE, + * after all servers are gone. */ + grpc_connectivity_state actual = sequences->connectivity_states[0]; + grpc_connectivity_state expected = GRPC_CHANNEL_READY; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), 0); + abort(); + } + + actual = sequences->connectivity_states[num_iters - 1]; + expected = GRPC_CHANNEL_TRANSIENT_FAILURE; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), (int)num_iters - 1); + abort(); } - /* even though we know all the servers are dead, the client is still trying - * retrying, believing it's in a transient failure situation */ - assert_channel_connectivity(client, 2, GRPC_CHANNEL_TRANSIENT_FAILURE, - GRPC_CHANNEL_CONNECTING); gpr_free(expected_connection_sequence); } @@ -826,15 +856,14 @@ static void dump_array(const char *desc, const int *data, const size_t count) { static void verify_rebirth_round_robin(const servers_fixture *f, grpc_channel *client, - const int *actual_connection_sequence, + const request_sequences *sequences, const size_t num_iters) { int *expected_connection_sequence; size_t i, j, unique_seq_last_idx, unique_seq_first_idx; const size_t expected_seq_length = f->num_servers; int *seen_elements; - dump_array("actual_connection_sequence", actual_connection_sequence, - num_iters); + dump_array("actual_connection_sequence", sequences->connections, num_iters); /* verify conn. seq. expectation */ /* get the first unique run of length "num_servers". */ @@ -845,13 +874,13 @@ static void verify_rebirth_round_robin(const servers_fixture *f, memset(seen_elements, 0, sizeof(int) * expected_seq_length); for (i = 0; i < num_iters; i++) { - if (actual_connection_sequence[i] < 0 || - seen_elements[actual_connection_sequence[i]] != 0) { + if (sequences->connections[i] < 0 || + seen_elements[sequences->connections[i]] != 0) { /* if anything breaks the uniqueness of the run, back to square zero */ memset(seen_elements, 0, sizeof(int) * expected_seq_length); continue; } - seen_elements[actual_connection_sequence[i]] = 1; + seen_elements[sequences->connections[i]] = 1; for (j = 0; j < expected_seq_length; j++) { if (seen_elements[j] == 0) break; } @@ -870,30 +899,71 @@ static void verify_rebirth_round_robin(const servers_fixture *f, unique_seq_first_idx = (unique_seq_last_idx - expected_seq_length + 1); memcpy(expected_connection_sequence, - actual_connection_sequence + unique_seq_first_idx, + sequences->connections + unique_seq_first_idx, sizeof(int) * expected_seq_length); /* first iteration succeeds */ - GPR_ASSERT(actual_connection_sequence[0] != -1); + GPR_ASSERT(sequences->connections[0] != -1); /* then we fail for a while... */ - GPR_ASSERT(actual_connection_sequence[1] == -1); + GPR_ASSERT(sequences->connections[1] == -1); /* ... but should be up at "unique_seq_first_idx" */ - GPR_ASSERT(actual_connection_sequence[unique_seq_first_idx] != -1); + GPR_ASSERT(sequences->connections[unique_seq_first_idx] != -1); for (j = 0, i = unique_seq_first_idx; i < num_iters; i++) { - const int actual = actual_connection_sequence[i]; + const int actual = sequences->connections[i]; const int expected = expected_connection_sequence[j++ % expected_seq_length]; if (actual != expected) { print_failed_expectations(expected_connection_sequence, - actual_connection_sequence, expected_seq_length, + sequences->connections, expected_seq_length, num_iters); abort(); } } - /* things are fine once the servers are brought back up */ - assert_channel_connectivity(client, 1, GRPC_CHANNEL_READY); + /* We can assert that the first client channel state should be READY, when all + * servers were available; same thing for the last one. In the middle + * somewhere there must exist at least one TRANSIENT_FAILURE */ + grpc_connectivity_state actual = sequences->connectivity_states[0]; + grpc_connectivity_state expected = GRPC_CHANNEL_READY; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), 0); + abort(); + } + + actual = sequences->connectivity_states[num_iters - 1]; + expected = GRPC_CHANNEL_READY; + if (actual != expected) { + gpr_log(GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: expected '%s', got '%s' " + "at iteration #%d", + grpc_connectivity_state_name(expected), + grpc_connectivity_state_name(actual), (int)num_iters - 1); + abort(); + } + + bool found_failure_status = false; + for (i = 1; i < sequences->n - 1; i++) { + if (sequences->connectivity_states[i] == GRPC_CHANNEL_TRANSIENT_FAILURE) { + found_failure_status = true; + } + } + if (!found_failure_status) { + gpr_log( + GPR_ERROR, + "CONNECTIVITY STATUS SEQUENCE FAILURE: " + "GRPC_CHANNEL_TRANSIENT_FAILURE status not found. Got the following " + "instead:"); + for (i = 0; i < num_iters; i++) { + gpr_log(GPR_ERROR, "[%d]: %s", (int)i, + grpc_connectivity_state_name(sequences->connectivity_states[i])); + } + } + gpr_free(expected_connection_sequence); gpr_free(seen_elements); } @@ -934,7 +1004,7 @@ int main(int argc, char **argv) { * This should knock down the server bound to be selected next */ test_spec_reset(spec); spec->verifier = verify_vanishing_floor_round_robin; - spec->description = "test_kill_all_server_at_2nd_iteration"; + spec->description = "test_kill_middle_servers_at_2nd_iteration"; for (i = 1; i < NUM_SERVERS - 1; i++) { spec->kill_at[1][i] = 1; } From ffd8e3d8e1397bcae315185818cd9d4784092682 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 15 Nov 2016 09:46:32 -0800 Subject: [PATCH 6/6] pr comment --- test/core/client_channel/lb_policies_test.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/core/client_channel/lb_policies_test.c b/test/core/client_channel/lb_policies_test.c index f4779d39351..c373505dd76 100644 --- a/test/core/client_channel/lb_policies_test.c +++ b/test/core/client_channel/lb_policies_test.c @@ -950,6 +950,7 @@ static void verify_rebirth_round_robin(const servers_fixture *f, for (i = 1; i < sequences->n - 1; i++) { if (sequences->connectivity_states[i] == GRPC_CHANNEL_TRANSIENT_FAILURE) { found_failure_status = true; + break; } } if (!found_failure_status) {