Use the RECV_STATUS op in ALTS handshake RPCs by apolcyn · Pull Request #20929 · grpc/grpc

apolcyn · 2019-11-05T04:53:55Z

This is a pre-req to #20722, which needs to make use of the RECV_STATUS op in order to manage a global queue. Note that though #20722 is meant to be a short-term change, this change is meant to be long term (the ALTS handshake RPC is currently somewhat unique in not doing this op).

Also, this is a replacement of #20687 (putting an explicit deadline on ALTS handshake RPCs is likely unnecessary, since a customized reconnect backoff time can serve that same purpose).

Some changes had to be made to the alts_handshaker_client_test and alts_tsi_handshaker_tests in order to make room for the new internal behaviors of alts_grpc_handshaker_client regarding lifetimes and how exactly the TSI next callback to be invoked.

This change is

apolcyn · 2019-11-06T09:17:24Z

Recent updates fix a bug that this change originally introduced:

Before this change, when the security handshaker's peer endpoint read failed, it would fail the subchannel connect attempt and destruct its tsi_handshaker object, but it would not call tsi_handshaker_shutdown. This was still ok with ALTS handshakes because the alts_tsi_handshaker object's dtor ran grpc_call_unref, which cancelled any ongoing handshake RPC and free'd up associated resources.

With this change, the alts_tsi_handshaker dtor no longer runs grpc_call_unref it there is an outstanding call that's waiting for a RECV_STATUS op to complete - instead, it just drops ref and defers full destruction to the RECV_STATUS op completion. This introduced a problem because if the security handshaker failed the subchannel connect attempt but didn't call tsi_handshaker_shutdown, then we wouldn't cancel any ongoing handshake RPC (previously, grpc_call_unref would do this cancellation). So recent changes change the security handshaker to be sure to call tsi_handshaker_shutdown if the security handshaker is failing the handshake attempt and the security handshaker hasn't been shut down yet; the issue could also be fixed perhaps by making the alts_tsi_handshaker dtor cancel any ongoing call, but the change in security handshaker seemed more predictable.

This still ready for review otherwise

markdroth

This looks pretty good. My comments are fairly minor.

Please make sure @yihuazhang reviews before merging.

Reviewed 4 of 6 files at r1, 2 of 4 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @apolcyn and @jiangtaoli2016)

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 54 at r1 (raw file):

  /* One ref is held by the entity that created this handshaker_client, and
   * another ref is held by the pending RECEIVE_STATUS_ON_CLIENT op. */
  gpr_refcount refs;

@yihuazhang, I would really like to see this code converted to C++, so that we can make this a C++ class that inherits from grpc_core::InternallyRefCounted<>.

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 156 at r3 (raw file):

      return;
    }
    bool have_final_result = false;

Could just say:

const bool have_final_result =
    client->pending_recv_message_result->result != nullptr ||
    client->pending_recv_message_result->status != TSI_OK;

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 192 at r3 (raw file):

}

static void handle_response_done_locked(alts_grpc_handshaker_client* client,

It looks like this is invoked when not holding the lock, so I think the _locked suffix should be removed.

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 300 at r3 (raw file):

    }
  }
  handle_response_done_locked(client,

Do we need to check for the recv_trailing_metadata response in this case? This isn't a failure, so presumably the handshaker's next() will be called again, right?

I'm thinking that when we know this is not going to be the last response (i.e., recv_buffer is not null), it would be more efficient to just invoke the callback directly, so that we don't suffer the overhead of the memory allocation and acquiring the mutex for any but the last read on the stream.

test/core/tsi/alts/handshaker/alts_concurrent_connectivity_test.cc, line 86 at r3 (raw file):

  // The main goal of these tests are to stress concurrent ALTS handshakes,
  // so we prevent subchnannel sharing.
  grpc_arg disable_subchannel_sharing_arg = grpc_channel_arg_integer_create(

Would be clearer written as follows:

std::vector<grpc_arg> new_args;
new_args.push_back(grpc_channel_arg_integer_create(
    const_cast<char*>(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), true));
if (reconnect_backoff_ms != 0) {
  new_args.push_back(grpc_channel_arg_integer_create(
      const_cast<char*>("grpc.testing.fixed_reconnect_backoff_ms"),
      reconnect_backoff_ms));
}
grpc_channel_args* channel_args = grpc_channel_args_copy_and_add(
    nullptr, new_args.data(), new_args.size());

apolcyn

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @apolcyn, @jiangtaoli2016, and @markdroth)

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 156 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Could just say:

const bool have_final_result =
    client->pending_recv_message_result->result != nullptr ||
    client->pending_recv_message_result->status != TSI_OK;

Done.

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 192 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

It looks like this is invoked when not holding the lock, so I think the _locked suffix should be removed.

Whoops, sorry, this was left over from earlier iteration that used the lock more widely

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 300 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Do we need to check for the recv_trailing_metadata response in this case? This isn't a failure, so presumably the handshaker's next() will be called again, right?

I'm thinking that when we know this is not going to be the last response (i.e., recv_buffer is not null), it would be more efficient to just invoke the callback directly, so that we don't suffer the overhead of the memory allocation and acquiring the mutex for any but the last read on the stream.

Unfortunately, this my or may not actually be the last response message, i.e. this response message can be the one that completes the handshake (in which case tsi_handshaker_next won't be called again). In a successful handshake, the last response message received from the handshake server contains a HandshakerResult field, which the client forms a tsi_handshaker_result from, and which terminates the handshake.

Note that assumptions this PR makes are:

We won't call tsi_handshaker_next again if we complete the current next callback with a non-null tsi_handshaker_result object
We won't call tsi_handshaker_next again if we complete the current next callback with tsi_result that is not TSI_OK (this scenario would indicate some failure in the handshake RPC)

Now that I'm describing this, I'm realizing there's one thing which I don't think is necessarily a bug but is potentially fragile: is we call back with nullptr tsi_handshaker_resultbut aTSI_INCOMPLETE_DATAstatus code, then we actually <i>aren't</i> done with the handshake; and so doing what this PR is doing, waiting for a status code, would be incorrect. AFAIKTSI_INCOMPLETE_DATA` is not a status code that will be generated by ALTS handshake servers though.

yihuazhang · 2019-11-07T20:42:24Z

@markdroth Sorry that I could not spend any time on C++'fying this class. I will squeeze it into my Q4 (or Q1) OKR.

apolcyn

Reviewable status: 5 of 7 files reviewed, 4 unresolved discussions (waiting on @apolcyn, @jiangtaoli2016, @markdroth, and @yihuazhang)

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 300 at r3 (raw file):
Re my own comment:

Now that I'm describing this, I'm realizing there's one thing which I don't think is necessarily a bug but is potentially fragile: is we call back with nullptr tsi_handshaker_resultbut aTSI_INCOMPLETE_DATAstatus code, then we actually aren't done with the handshake; and so doing what this PR is doing, waiting for a status code, would be incorrect. AFAIKTSI_INCOMPLETE_DATA` is not a status code that will be generated by ALTS handshake servers though.

I chatted with @jiangtao and @yihuaz offline, and audited the code. We have a guarantee that we will never invoke the tsi_handshaker_next callback with status == TSI_INCOMPLETE_DATA, because:

The ALTS handshaker client code will never generate a TSI_INCOMPLETE_DATA status on its own
b) There doesn't exist a status that the ALTS handshake server can return that can be converted to TSI_INCOMPLETE_DATA:

grpc/src/core/tsi/alts/handshaker/alts_tsi_utils.cc

Line 27 in 438cfa7

tsi_result alts_tsi_utils_convert_to_tsi_result(grpc_status_code code) {

So I still think that the current code in the PR should be stuck with.

I'm thinking that when we know this is not going to be the last response (i.e., recv_buffer is not null), it would be more efficient to just invoke the callback directly, so that we don't suffer the overhead of the memory allocation and acquiring the mutex for any but the last read on the stream.

Related to above comment about needing further inspection at this point to know whether or not we're done, and if next will be called again - we might be able to do some inspection at this point to do some short circuiting, e.g. check if result == nullptr or check if status != TSI_OK, but the advantage I like of the current code is that the logic of whether or not we're done yet is kept all within maybe_complete_tsi_next... PLMK though

markdroth

Looks good! Only minor comments remaining. Feel free to merge after addressing them.

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @apolcyn, @jiangtaoli2016, and @yihuazhang)

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 300 at r3 (raw file):

Previously, apolcyn wrote…

Re my own comment:

Now that I'm describing this, I'm realizing there's one thing which I don't think is necessarily a bug but is potentially fragile: is we call back with nullptr tsi_handshaker_resultbut aTSI_INCOMPLETE_DATAstatus code, then we actually aren't done with the handshake; and so doing what this PR is doing, waiting for a status code, would be incorrect. AFAIKTSI_INCOMPLETE_DATA` is not a status code that will be generated by ALTS handshake servers though.

I chatted with @jiangtao and @yihuaz offline, and audited the code. We have a guarantee that we will never invoke the tsi_handshaker_next callback with status == TSI_INCOMPLETE_DATA, because:

The ALTS handshaker client code will never generate a TSI_INCOMPLETE_DATA status on its own
b) There doesn't exist a status that the ALTS handshake server can return that can be converted to TSI_INCOMPLETE_DATA:

grpc/src/core/tsi/alts/handshaker/alts_tsi_utils.cc

Line 27 in 438cfa7

tsi_result alts_tsi_utils_convert_to_tsi_result(grpc_status_code code) {

So I still think that the current code in the PR should be stuck with.

I'm thinking that when we know this is not going to be the last response (i.e., recv_buffer is not null), it would be more efficient to just invoke the callback directly, so that we don't suffer the overhead of the memory allocation and acquiring the mutex for any but the last read on the stream.

Related to above comment about needing further inspection at this point to know whether or not we're done, and if next will be called again - we might be able to do some inspection at this point to do some short circuiting, e.g. check if result == nullptr or check if status != TSI_OK, but the advantage I like of the current code is that the logic of whether or not we're done yet is kept all within maybe_complete_tsi_next... PLMK though

I'm okay with this for now, but this does seem like it adds more overhead than is really necessary. Maybe add a TODO indicating that if this becomes a performance problem, we should find a way to avoid the extra overhead on all but the last message?

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 156 at r4 (raw file):

      return;
    }
    bool have_final_result =

This can be const.

apolcyn

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @jiangtaoli2016, @markdroth, and @yihuazhang)

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 300 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I'm okay with this for now, but this does seem like it adds more overhead than is really necessary. Maybe add a TODO indicating that if this becomes a performance problem, we should find a way to avoid the extra overhead on all but the last message?

Added this TODO

src/core/tsi/alts/handshaker/alts_handshaker_client.cc, line 156 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can be const.

Done.

test/core/tsi/alts/handshaker/alts_concurrent_connectivity_test.cc, line 86 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Would be clearer written as follows:

std::vector<grpc_arg> new_args;
new_args.push_back(grpc_channel_arg_integer_create(
    const_cast<char*>(GRPC_ARG_USE_LOCAL_SUBCHANNEL_POOL), true));
if (reconnect_backoff_ms != 0) {
  new_args.push_back(grpc_channel_arg_integer_create(
      const_cast<char*>("grpc.testing.fixed_reconnect_backoff_ms"),
      reconnect_backoff_ms));
}
grpc_channel_args* channel_args = grpc_channel_args_copy_and_add(
    nullptr, new_args.data(), new_args.size());

done (in prior update)

yihuazhang · 2019-11-08T21:37:52Z

src/core/tsi/alts/handshaker/alts_handshaker_client.cc

+      GPR_ASSERT(client->pending_recv_message_result == nullptr);
+      client->pending_recv_message_result = pending_recv_message_result;
+    }
+    if (client->pending_recv_message_result == nullptr) {


Does this branch handle the situation when ALTS handshaker server was unable to handle a client's requests and notify the handshaker client about it?

Yes, if I understand the question: if the ALTS handshake server was unable to handle the client's request and the ALTS handshake server was unable to notify the client about it, then what happens is the following:

pending RECV_MESSAGE op and pending RECV_STATUS_ON_CLIENT op will both hang

After subchannel connection timeout elapses, the subchannel's associated handshake manager will shut down the ongoing security handshake, which will invoke grpc_call_cancel_internal on the ALTS RPC

pending RECV_MESSAGE and RECV_STATUS_ON_CLIENT ops will then both complete

handle_response_done invoke this with a non-null pending_recv_message_result, having a status != TSI_OK

on_status_received will invoke with this with receive_status_finished

We'll then invoke the TSI next callback, and the handshake will be terminated.

Note that the added test in alts_concurrent_connectivity_test.cc: TestHandshakeFailsFastWhenHandshakeServerHangsAfterAccepting, exercises this case.

Thanks much for the explanation. It makes sense to me. Could you please also leave this conversation open for future reference?

np, will leave this open

yihuazhang · 2019-11-08T21:40:42Z

Hi Alex,

I am trying to understand the motivation of this PR. Does the PR handle the situation when ALTS handshake server was unable to handle handshake clients' requests (due to resource limit exceeding) and thus notify the client about it?

yihuazhang · 2019-11-08T21:59:04Z

src/core/tsi/alts/handshaker/alts_handshaker_client.cc

+    op++;
+    GPR_ASSERT(op - ops <= kHandshakerClientOpNum);
+    gpr_ref(&client->refs);
+    grpc_call_error call_error =


For the client's first handshake message, don't we endup calling TSI next callback twice? One is in on_status_received and the other in on_handshaker_service_resp_recv?

The client's first handshake message response, in successful case, should result in a received message having no result field, and an OK status. In this case, maybe_complete_tsi_next will not wait for the RECV_STATUS op to complete.

Eventually, the last handshaker message will arrive, having at least either non-OK status or a filled in result field. In this case, we'll wait for the handshake RPC's status to be received, and then invoke the TSI next callback just once.

Lastly, note that if the RECV_STATUS op completes while we don't have a TSI next callback outstanding (e.g. security handshaker is waiting for a read from remote peer), then we won't have a non-null pending_recv_message_result, and so we won't accidentally invoke the TSI next callback an extra time

In the last handshaker message, why do we wait for the handshake RPC's status to be received?

Please also leave this conversation open.

In the last handshaker message, why do we wait for the handshake RPC's status to be received?

My motivation for this is to mainly just to try to be sure that we continue to drive the handshake RPC forward until it fully completes. This is basically trying to leverage the fact that there is an outstanding TSI next call (which ultimately suggests that the associated subchannel hasn't yet connected, and that someone should be polling on the relevant file descriptors of this handshake RPC), to make sure that we finish the handshake RPC in a timely manner.

In a degenerate case, suppose that the handshake server sends a reply message, and so the RECV_MESSAGE op completes, but the data on the wire hasn't yet arrived to complete the RECV_STATUS op. If we completed the TSI handshake at this point, then the subchannel could become ready, the application could quickly complete its RPC, and then the application could stop polling on the completion queue of its RPC (if it e.g. only wanted to do a single RPC). The data necessary to complete the RECV_STATUS op on the wire could then show up just after, but because we don't have anyone guaranteed to be polling on the relevant file descriptors of the handshake RPC (except for the optional backup poller, which runs on a fixed time interval), the handshake RPC may just hang there and never "complete".

I.e., this provides a kind of guarantee that all handshake RPCs are either driven forward until full completion in a timely manner, or be cancelled (due to handshake failure or subchannel connection timeout).

Thanks much for the explanation. It makes sense to me.

In xds_cleint.cc (https://github.com/grpc/grpc/blob/master/src/core/ext/filters/client_channel/xds/xds_client.cc), we also applied the similar model, i.e., scheduling a grpc_closure upon receiving message RECV_MESSAGE and status RECV_STATUS.

apolcyn · 2019-11-08T22:44:44Z

I am trying to understand the motivation of this PR. Does the PR handle the situation when ALTS handshake server was unable to handle handshake clients' requests (due to resource limit exceeding) and thus notify the client about it?

The immediate motivation is to prepare for #20722

There is a specific case in #20722 PR (the global queue), that I'm using the RECV_STATUS op to handle:

Specifically, that PR moves the global handshake RPC queue forward as follows:

when making a new handshake RPC, first check if there's space outstanding in the queue, and enqueue if there isn't
when completing a handshake RPC, check if there's any handshake waiting for an available "outstanding handshake RPC" slot to become available, and start it if so.

So if the security handshaker is e.g. waiting for a read from the peer, while not having an outstanding TSI next call, then we still need a way to move the queue forward after we know that the current handshake RPC is done, the RECV_STATUS op's completion is IMO a convenient way to do this.

Also, #20722 aside, the handshake RPC is the currently special in that it's the only RPC that I know of that doesn't use the RECV_STATUS op. This is likely error prone due to being abnormal usage of the core call API (FWIW I believe this also technically not conforming to C-core API rules - https://github.com/grpc/grpc/blob/master/include/grpc/impl/codegen/grpc_types.h#L572).

yihuazhang · 2019-11-08T23:34:18Z

src/core/tsi/alts/handshaker/alts_handshaker_client.cc

  alts_handshaker_client base;
+  /* One ref is held by the entity that created this handshaker_client, and
+   * another ref is held by the pending RECEIVE_STATUS_ON_CLIENT op. */
+  gpr_refcount refs;


So I believe by having this refs, we can correctly handle the situation when alts_tsi_handshaker gets destroyed while we still have a pending RECEIVE_STATUS_ON_CLIENT op?

Exactly, correct

yihuazhang · 2019-11-08T23:58:08Z

src/core/tsi/alts/handshaker/alts_handshaker_client.cc

+    const bool have_final_result =
+        client->pending_recv_message_result->result != nullptr ||
+        client->pending_recv_message_result->status != TSI_OK;
+    if (have_final_result && !client->receive_status_finished) {


Could you please add some comments about this line? e.g., it returns when 1) handshake successfully completes (client->pending_recv_message_result->result != nullptr) or 2) handshake fails (client->pending_recv_message_result->status != TSI_OK) or client has not received response on client's RECV_STATUS_ON_CLIENT.

It seems that we will not run TSI next callback when 1) Handshake completes and succeeds (client->pending_recv_message_result->result != nullptr) or handshake fails (client->pending_recv_message_result->status != TSI_OK) and 2) we do not receive the response on client's RECV_STATUS_ON_CLIENT op. Is it because if handshake server returns response for RECV_MESSAGE, it will not return for RECV_STATUS_ON_CLIENT? Also, if the handshake fails, should not we also invoke the TSI next callback by feeding in corresponding error status?

Please ignore my second question. I think your response to my questions below (i.e., calling TSI next callback twice) answers it :)

Could you please add some comments about this line? e.g., it returns when 1) handshake successfully completes (client->pending_recv_message_result->result != nullptr) or 2) handshake fails (client->pending_recv_message_result->status != TSI_OK) or client has not received response on client's RECV_STATUS_ON_CLIENT.

Yes, we return early here if "handshake successfully completes or handshake fails" and "not received status'". I added this comment

Also, if the handshake fails, should not we also invoke the TSI next callback by feeding in corresponding error status?

I do think it would make sense to override an otherwise-OK TSI next status with an error if the RECV_STATUS op indicates that the handshake RPC has failed. I've left a TODO for this though; if doing this, propose saving it for another PR.

yihuazhang · 2019-11-09T01:03:07Z

LGTM. Please feel free to merge after addressing my last comment on "why waiting for the handshake RPC's status to be received for the last handshake message".

apolcyn · 2019-11-09T02:06:35Z

Thanks for the detailed reviews @markdroth and @yihuazhang . Will merge on monday.

apolcyn · 2019-11-11T19:00:10Z

Squashed down commits into one.

Second commit is a trivial post-review change to change a CLOSURE_RUN to a Closure::Run

apolcyn · 2019-11-11T22:51:46Z

Test failures:

objc_macos_opt_native: #20686
Bazel RBE ASAN C++: #15981
Basic Tests C/C++ Macos: #20694
Bazel RBE MSAN C/C++: #21013
Bazel RBE Opt C/C++: #20759 (xds_end2end_test exit code 142)
Interop cloud to prod failed with: fatal: unable to access 'https://go.googlesource.com/text/': The requested URL returned error: 500
RBE Windows Debug C/C++: "Tool Failed" RBE failure
UBSAN C/C++: #21014

apolcyn added area/security lang/core release notes: no Indicates if PR should not be in release notes labels Nov 5, 2019

apolcyn force-pushed the alts_recv_status branch from 5688d3b to 35ff8e3 Compare November 5, 2019 09:40

apolcyn mentioned this pull request Nov 5, 2019

Put a 2-second deadline on ALTS handshake RPCs #20687

Closed

apolcyn marked this pull request as ready for review November 5, 2019 09:44

apolcyn assigned yihuazhang and markdroth and unassigned yihuazhang Nov 5, 2019

apolcyn requested a review from jiangtaoli2016 November 5, 2019 09:45

apolcyn force-pushed the alts_recv_status branch from e195e24 to d09b233 Compare November 6, 2019 04:47

markdroth requested changes Nov 7, 2019

View reviewed changes

apolcyn commented Nov 7, 2019

View reviewed changes

yihuazhang self-requested a review November 7, 2019 20:37

apolcyn commented Nov 7, 2019

View reviewed changes

markdroth approved these changes Nov 8, 2019

View reviewed changes

apolcyn commented Nov 8, 2019

View reviewed changes

yihuazhang reviewed Nov 8, 2019

View reviewed changes

yihuazhang approved these changes Nov 11, 2019

View reviewed changes

apolcyn force-pushed the alts_recv_status branch from f816c03 to 06035ce Compare November 11, 2019 18:51

apolcyn added 2 commits November 11, 2019 10:54

Have ALTS handshake clients do a RECV_STATUS op

d7c51c6

Update CLOSURE_RUN to Closure::Run

8e9c5a4

apolcyn force-pushed the alts_recv_status branch from 06035ce to 8e9c5a4 Compare November 11, 2019 18:59

apolcyn merged commit 8c98066 into grpc:master Nov 11, 2019

jtattermusch mentioned this pull request Nov 13, 2019

alts_concurrent_connectivity_test is failing consistently on master #21142

Closed

hcaseyal mentioned this pull request Nov 13, 2019

Revert "Use the RECV_STATUS op in ALTS handshake RPCs" #21189

Closed

yihuazhang mentioned this pull request Dec 18, 2019

S2A handshaker client, part 2 matthewstevenson88/grpc#23

Merged

lock bot locked as resolved and limited conversation to collaborators Feb 10, 2020

Conversation

apolcyn commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apolcyn commented Nov 6, 2019

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

apolcyn left a comment

Choose a reason for hiding this comment

Uh oh!

yihuazhang commented Nov 7, 2019

Uh oh!

apolcyn left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

apolcyn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuazhang commented Nov 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apolcyn commented Nov 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuazhang commented Nov 9, 2019

Uh oh!

apolcyn commented Nov 9, 2019

Uh oh!

apolcyn commented Nov 11, 2019

Uh oh!

apolcyn commented Nov 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

apolcyn commented Nov 5, 2019 •

edited

Loading

apolcyn commented Nov 8, 2019 •

edited

Loading