quic: add QUIC downstream connection close error stats. by RenjieTang · Pull Request #16584 · envoyproxy/envoy

RenjieTang · 2021-05-19T21:33:14Z

Signed-off-by: Renjie Tang renjietang@google.com

Commit Message: Introduce QuicStatNames and add Quic connection close stats for downstream connections.
Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang · 2021-05-19T21:33:39Z

/assign @alyssawilk
Hi Alyssa, would you mind taking a first pass review on this PR?
I think currently this change works well with H3 enabled. But I’m having trouble making it work without H3.
Since QuicStats exists as class members and is passed down in constructors, is there any good way to avoid the build dependency when H3 is disabled? I could imagine adding extra constructors, but not sure if that’s the right approach.

alyssawilk

Hm.

At first pass my thought was that all our existing http3 stats are rooted in the HCM, and it'd be nice to be consistent here.
I'm not sure if that's the right thing here, because of the L4/L7 thing, but given that for TCP we don't have the stats in the connection, but we have the tcp network filter or the HCM do the accounting, I think a utility we can reuse when we add other protocols plus wiring it up in the HCM might be the right thing. Tagging @jmarantz for a second opinion as stats are more his area.

If we go that route, I think we could stash the close details in connection level stream info as one of those generic extension points, and pull it out in the HCM if it's present. We've already got a few h3 stats in the HCM which we don't bother compiling out, so as long as we added lazily I think it wouldn't be a problem.

RenjieTang · 2021-05-20T20:29:16Z

I saw that HttpConnectionManagerConfig owns http3_codec_stats_ and passes it down to QuicHttpServerConnectionImpl.
Maybe we can have QuicStats to follow the same pattern?

jmarantz · 2021-05-21T02:04:51Z

clang-tidy error needs attention:

2021-05-19T22:37:06.0307772Z Writing fixes to clang-tidy-fixes.yaml ...
2021-05-19T22:37:06.0815884Z clang-tidy check failed, potentially fixed by clang-apply-replacements:
2021-05-19T22:37:06.0853537Z Diagnostics:
2021-05-19T22:37:06.0854670Z - BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
2021-05-19T22:37:06.0855193Z   DiagnosticMessage:
2021-05-19T22:37:06.0855548Z     FileOffset: 619
2021-05-19T22:37:06.0856073Z     FilePath: source/common/quic/quic_stats.h
2021-05-19T22:37:06.0856758Z     Message: invalid case style for function 'ConnectionCloseStatName'
2021-05-19T22:37:06.0857206Z     Replacements:
2021-05-19T22:37:06.0857930Z     - {FilePath: source/common/quic/quic_stats.h, Length: 23, Offset: 619, ReplacementText: connectionCloseStatName}
2021-05-19T22:37:06.0858685Z   DiagnosticName: readability-identifier-naming
2021-05-19T22:37:06.0859093Z   Level: Warning
2021-05-19T22:37:06.0859745Z - BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
2021-05-19T22:37:06.0864759Z   DiagnosticMessage:
2021-05-19T22:37:06.0865156Z     FileOffset: 3737
2021-05-19T22:37:06.0865566Z     FilePath: source/common/quic/active_quic_listener.h
2021-05-19T22:37:06.0866392Z     Message: invalid case style for parameter 'quic_stats_'
2021-05-19T22:37:06.0866825Z     Replacements:
2021-05-19T22:37:06.0867492Z     - {FilePath: source/common/quic/active_quic_listener.h, Length: 11, Offset: 3737,
2021-05-19T22:37:06.0868006Z       ReplacementText: quic_stats}
2021-05-19T22:37:06.0868748Z   DiagnosticName: readability-identifier-naming
2021-05-19T22:37:06.0869157Z   Level: Warning
2021-05-19T22:37:06.0869634Z MainSourceFile: ''
2021-05-19T22:37:06.3876930Z ##[error]Bash exited with code '1'.
2021-05-19T22:37:06.4036353Z ##[section]Finishing: Run CI script

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang · 2021-05-21T04:00:17Z

clang-tidy error needs attention:

2021-05-19T22:37:06.0307772Z Writing fixes to clang-tidy-fixes.yaml ...
2021-05-19T22:37:06.0815884Z clang-tidy check failed, potentially fixed by clang-apply-replacements:
2021-05-19T22:37:06.0853537Z Diagnostics:
2021-05-19T22:37:06.0854670Z - BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
2021-05-19T22:37:06.0855193Z   DiagnosticMessage:
2021-05-19T22:37:06.0855548Z     FileOffset: 619
2021-05-19T22:37:06.0856073Z     FilePath: source/common/quic/quic_stats.h
2021-05-19T22:37:06.0856758Z     Message: invalid case style for function 'ConnectionCloseStatName'
2021-05-19T22:37:06.0857206Z     Replacements:
2021-05-19T22:37:06.0857930Z     - {FilePath: source/common/quic/quic_stats.h, Length: 23, Offset: 619, ReplacementText: connectionCloseStatName}
2021-05-19T22:37:06.0858685Z   DiagnosticName: readability-identifier-naming
2021-05-19T22:37:06.0859093Z   Level: Warning
2021-05-19T22:37:06.0859745Z - BuildDirectory: /build/tmp/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/execroot/envoy
2021-05-19T22:37:06.0864759Z   DiagnosticMessage:
2021-05-19T22:37:06.0865156Z     FileOffset: 3737
2021-05-19T22:37:06.0865566Z     FilePath: source/common/quic/active_quic_listener.h
2021-05-19T22:37:06.0866392Z     Message: invalid case style for parameter 'quic_stats_'
2021-05-19T22:37:06.0866825Z     Replacements:
2021-05-19T22:37:06.0867492Z     - {FilePath: source/common/quic/active_quic_listener.h, Length: 11, Offset: 3737,
2021-05-19T22:37:06.0868006Z       ReplacementText: quic_stats}
2021-05-19T22:37:06.0868748Z   DiagnosticName: readability-identifier-naming
2021-05-19T22:37:06.0869157Z   Level: Warning
2021-05-19T22:37:06.0869634Z MainSourceFile: ''
2021-05-19T22:37:06.3876930Z ##[error]Bash exited with code '1'.
2021-05-19T22:37:06.4036353Z ##[section]Finishing: Run CI script

done

jmarantz

This generally looks great! I just have a few comments about making the stats contexts parallel those for HTTP and GRPC for increased consistency and reduced number of args at various interface-points.

jmarantz · 2021-05-21T02:16:53Z

source/common/quic/quic_stats.cc

+                     connection_close});
+}
+
+Stats::StatName QuicStats::ConnectionCloseStatName(quic::QuicErrorCode error_code) {


connectionCloseStatName per Envoy style guide

jmarantz · 2021-05-21T11:44:12Z

source/server/server.cc


    restarter_.initialize(*dispatcher_, *this);
    drain_manager_ = component_factory.createDrainManager(*this);
+#ifdef ENVOY_ENABLE_QUIC


is there also a runtime flag that should be checked? Or it's ifdef-only?

The ifdef alone should suffice.

Is there a configuration option for Quic? Or you have to make the decision about whether to support it at compile-time?

The QUIC support is decided at compile time and is controlled by --//bazel:http3=True/False

jmarantz · 2021-05-21T11:48:20Z

source/server/listener_impl.h

  // callback during the destroy of ListenerImpl.
  Init::WatcherImpl local_init_watcher_;

+  Quic::QuicStats* quic_stats_;


Envoy style here is to use OptRef<Quic::QuicStats> here rather than a possibly null pointer.

See include/envoy/common/optref.h

jmarantz · 2021-05-21T12:13:51Z

source/common/quic/active_quic_listener.cc

    Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config,
    Network::Socket::OptionsSharedPtr options, bool kernel_worker_routing,
-    const envoy::config::core::v3::RuntimeFeatureFlag& enabled)
+    const envoy::config::core::v3::RuntimeFeatureFlag& enabled, QuicStats* quic_stats)


I'd suggest bundling QuicStats and quic::QuicConfig into a class Quic::Context, which can parallel Http::Context in most ways.

That will cut down by one on the arg-count in a few interfaces in this PR, and give you other good places to put stuff that may need to be shared across workers.

Moreover, Quic::Context (or maybe OptRef<Quic::Context>) could then be returned from Server::FactoryContext, also parallel to Http::Context and Grpc::Context, cutting down on how much plumbing through needs to happen for those individually.

I originally tried to make QuicStats behave similarly to Http::Context but found it to be quite complicated.

To have the server return QuicStats, we need a pure interface class so that Server::Instance can have a getter interface Quic::QuicStats quicStats().
However, the only public method in QuicStats, chargeQuicConnectionCloseStats(), is dependent on QuicErrorCode, which is defined in third_party library Quiche.
In my understanding, Envoy must to be able to build without quiche. And we probably shouldn't introduce external dependency in the include/ directory either.
Having this dependency makes it hard to follow existing pattern when implementing Quic stats.

On this point, to match the pattern in the other Contexts is to forward-declare QuicStats without defining an interface for it.

So you'd have

namespace quic { class QuicConfig; } namespace Envoy { namespace Quic { class QuicStats; class Context { public: virtual QuicStats& stats() PURE: virtual quic::QuicConfig& config() PURE; }; ...

So you don't need to expose the error-code type or other types at the interface.

jmarantz · 2021-05-21T12:28:04Z

source/common/quic/envoy_quic_dispatcher.cc

    Network::ListenerConfig& listener_config, Server::ListenerStats& listener_stats,
    Server::PerHandlerListenerStats& per_worker_stats, Event::Dispatcher& dispatcher,
-    Network::Socket& listen_socket)
+    Network::Socket& listen_socket, QuicStats* quic_stats)


per above, pass through QuicContext rather than QuicStats and QuicConfig separately.

jmarantz · 2021-05-21T12:28:47Z

source/common/quic/envoy_quic_dispatcher.cc

  listener_stats_.downstream_cx_active_.dec();
  per_worker_stats_.downstream_cx_active_.dec();
  connection_handler_.decNumConnections();
+  if (quic_stats_) {


I think you should just make QuicStats unconditional at this level. I don't see the problem with instantiating QuicStats in the places below where you are currently passing nullptr.

RenjieTang

Thanks for the review Joshua! Glad I got most of the QuicStats class right.
I left some in-place comments on the ownership and plumbing-through of QuicStats.
In general, I think it might not be the best choice to have the Server::InstanceImpl own QuicStats and pass it down all the way.

Alyssa mentioned in comments that a lot of existing QUIC stats live in HttpConnectionManagerConfig. I tried placing QuicStats there and found things to be a lot cleaner. The demo PR can be found at #16625. Let me know what you think on that.

RenjieTang · 2021-05-21T20:56:04Z

source/common/quic/active_quic_listener.cc

    Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config,
    Network::Socket::OptionsSharedPtr options, bool kernel_worker_routing,
-    const envoy::config::core::v3::RuntimeFeatureFlag& enabled)
+    const envoy::config::core::v3::RuntimeFeatureFlag& enabled, QuicStats* quic_stats)


I originally tried to make QuicStats behave similarly to Http::Context but found it to be quite complicated.

To have the server return QuicStats, we need a pure interface class so that Server::Instance can have a getter interface Quic::QuicStats quicStats().
However, the only public method in QuicStats, chargeQuicConnectionCloseStats(), is dependent on QuicErrorCode, which is defined in third_party library Quiche.
In my understanding, Envoy must to be able to build without quiche. And we probably shouldn't introduce external dependency in the include/ directory either.
Having this dependency makes it hard to follow existing pattern when implementing Quic stats.

RenjieTang · 2021-05-21T20:59:39Z

source/server/server.cc


    restarter_.initialize(*dispatcher_, *this);
    drain_manager_ = component_factory.createDrainManager(*this);
+#ifdef ENVOY_ENABLE_QUIC


The ifdef alone should suffice.

jmarantz

I'll check out the other PR a bit later.

jmarantz · 2021-05-21T22:57:46Z

source/common/quic/active_quic_listener.cc

    Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config,
    Network::Socket::OptionsSharedPtr options, bool kernel_worker_routing,
-    const envoy::config::core::v3::RuntimeFeatureFlag& enabled)
+    const envoy::config::core::v3::RuntimeFeatureFlag& enabled, QuicStats* quic_stats)


On this point, to match the pattern in the other Contexts is to forward-declare QuicStats without defining an interface for it.

So you'd have

namespace quic { class QuicConfig; } namespace Envoy { namespace Quic { class QuicStats; class Context { public: virtual QuicStats& stats() PURE: virtual quic::QuicConfig& config() PURE; }; ...

So you don't need to expose the error-code type or other types at the interface.

jmarantz · 2021-05-21T22:59:30Z

In general, I think it might not be the best choice to have the Server::InstanceImpl own QuicStats and pass it down all the way.

Can you explain the reasoning?

RenjieTang · 2021-05-21T23:20:39Z

In general, I think it might not be the best choice to have the Server::InstanceImpl own QuicStats and pass it down all the way.

Can you explain the reasoning?

I was mainly concerned that having QuicContext accessible in Server::Instance will require getter implementation in subclasses that don't really need it, for example Server::ValidationInstance. If having QuicStats in HCM works just as well, I'd prefer to go that route so that the code is cleaner.

ListenerManagerImpl. Signed-off-by: Renjie Tang <renjietang@google.com>

Signed-off-by: Renjie Tang <renjietang@google.com>

jmarantz

this is looking much better. I think we can just get rid of the one OptRef.

source/server/listener_impl.cc

source/server/listener_impl.h

jmarantz · 2021-05-25T17:22:19Z

source/server/listener_manager_impl.cc

  }

+#ifdef ENVOY_ENABLE_QUIC
+  new_listener->setQuicStatNames(quic_stat_names_);


As above I think ListenerImpl can just grab the QuicStatNames& object from the manager.

Or...if that involves changes to the base class you'd prefer not to do, you can just pass in the stats to the constructor rather than setting it after construction.

That just means you need to ifdef the arg-list.

Fortunately ListenerImpl takes ListenerManagerImpl in its constructors. So I added getter in ListenerManagerImpl and let ListenerImpl grab it during constructions.

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang · 2021-05-26T16:49:02Z

/retest

repokitteh-read-only · 2021-05-26T16:49:05Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16584 (comment) was created by @RenjieTang.

see: more, trace.

jmarantz

@envoyproxy/senior-maintainers

thanks. for pushing this all the way through. nice job!

jmarantz · 2021-05-28T00:54:02Z

/assign-from @envoyproxy/senior-maintainers

alyssawilk

Awesome - these are going to be super useful!

source/common/quic/quic_stat_names.cc

test/common/quic/quic_stat_names_test.cc

source/common/quic/quic_stat_names.h

test/common/quic/quic_stat_names_test.cc

Signed-off-by: Renjie Tang <renjietang@google.com>

alyssawilk

One naming nit, and I think this is good to go from my point of view.

Adding Matt for a non-google pass since stats are forever :-P

alyssawilk · 2021-06-01T21:05:34Z

docs/root/configuration/http/http_conn_man/stats.rst

+   :header: Name, Type, Description
+   :widths: 1, 1, 2
+
+   [direction].[source].quic_connection_close_error_code_[error_code], Counter, A collection of counters that are lazily initialized to record each quic connection close error code that's present. direction could be *upstream* or *downstream*. source could be *self* or *peer*.


If we're doccing up listener stats, won't direction always be downstream?

And actually I hate to catch this now, but probably rather than self/peer it should be tx/rx for consistency with other Envoy stats. I find self and peer more intuitive (maybe 'cause that's what I'm used to) but I think most of the other stats (like resets tx_reset/rx_reset) are documented that way with tx for things sent to the peer and rx being codes received from the peer.

You are right. The listener stats should be all downstreams.

I haven't figured out the details on upstream stats. So I will update the doc again when I add them.

Done changing names to tx/rx.

Signed-off-by: Renjie Tang <renjietang@google.com>

mattklein123 · 2021-06-02T15:25:34Z

Check CI?

/wait

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang · 2021-06-02T20:58:02Z

@mattklein123 The flaky tests are now fixed. PTAL.

mattklein123

Thanks LGTM with some small comments.

/wait

docs/root/configuration/http/http_conn_man/stats.rst

mattklein123 · 2021-06-02T21:46:41Z

source/common/quic/quic_stat_names.cc

+// TODO(renjietang): Currently these stats are only available in downstream. Wire it up to upstream
+// QUIC also.


From a quick read through it's not clear to me that this won't create an unbounded number of stats controlled by downstream (for exampled if the name somehow has the code number in it and the client controls the codes). This is probably not the case, but can you add more comments about why this is safe for use with uncontrolled peers?

Great point!
I initially had an assertion in QuicStatNames::connectionCloseStatName() to make sure we don't create unlimited amount of stats.
But after some investigation, I found that if the connection close is initiated from peers, the error_code is parsed from the wire and no enum range checking is done. So an assertion here is too strong and malicious clients might be able to attack Envoy by sending out-of-range error codes.

Instead, I now added a check in QuicStatNames::chargeQuicConnectionCloseStats() to ignore out-of-range error_codes and log a warning.

I will follow up to dig deeper into the quiche code and see if bad error code handling can be done earlier.

Signed-off-by: Renjie Tang <renjietang@google.com>

mattklein123 · 2021-06-02T22:28:55Z

source/common/quic/quic_stat_names.cc

  ASSERT(&symbol_table_ == &scope.symbolTable());

+  if (error_code >= quic::QUIC_LAST_ERROR) {
+    ENVOY_LOG(warn, fmt::format("Error code {} is out of range of QuicErrorCodes.", error_code));


Can you add a test case for this? Also, I think warning here can log spam. I would recommend making this debug level and potentially also adding a stat for unknown error code or something like that. Thank you.

/wait

done.
I utilized quic::QUIC_LAST_ERROR_CODE as a stat for all out of range codes.

Signed-off-by: Renjie Tang <renjietang@google.com>

mattklein123

Thanks!

…6584) Signed-off-by: Renjie Tang <renjietang@google.com>

Add QUIC downstream connection close error stats.

b7b52e1

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot assigned alyssawilk May 19, 2021

alyssawilk reviewed May 20, 2021

View reviewed changes

alyssawilk added the waiting label May 20, 2021

alyssawilk assigned jmarantz May 20, 2021

change naming style

a704f7d

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot removed the waiting label May 21, 2021

more naming change

e77ca28

Signed-off-by: Renjie Tang <renjietang@google.com>

jmarantz reviewed May 21, 2021

View reviewed changes

RenjieTang commented May 21, 2021

View reviewed changes

jmarantz reviewed May 21, 2021

View reviewed changes

RenjieTang added 2 commits May 25, 2021 00:11

Change QuicStats to QuicStatNames and move its ownership to

1b9275b

ListenerManagerImpl. Signed-off-by: Renjie Tang <renjietang@google.com>

Merge remote-tracking branch 'upstream/main' into quic-stats

a47dc19

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang mentioned this pull request May 25, 2021

quic: add quic downstream stats in hcm #16625

Closed

fix format

c4394c5

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang requested a review from jmarantz May 25, 2021 17:11

jmarantz reviewed May 25, 2021

View reviewed changes

RenjieTang added 2 commits May 25, 2021 11:25

Remove OptRef and set quic_stat_names_ in constructors.

71f28c3

Signed-off-by: Renjie Tang <renjietang@google.com>

format

a71c23c

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang requested a review from jmarantz May 25, 2021 18:32

jmarantz previously approved these changes May 27, 2021

View reviewed changes

alyssawilk reviewed Jun 1, 2021

View reviewed changes

alyssawilk added the waiting label Jun 1, 2021

RenjieTang added 3 commits June 1, 2021 13:23

add more tests

8957117

Signed-off-by: Renjie Tang <renjietang@google.com>

Merge remote-tracking branch 'upstream/main' into quic-stats

5c83f5f

Signed-off-by: Renjie Tang <renjietang@google.com>

address Alyssa's comments.

6cd0d83

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot removed the waiting label Jun 1, 2021

alyssawilk reviewed Jun 1, 2021

View reviewed changes

alyssawilk assigned mattklein123 Jun 1, 2021

RenjieTang added 2 commits June 1, 2021 14:18

update stat name

4fe3fc5

Signed-off-by: Renjie Tang <renjietang@google.com>

doc formatting

ac867ce

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot added the waiting label Jun 2, 2021

attempt to fix flaky tests

d009105

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot removed the waiting label Jun 2, 2021

alyssawilk previously approved these changes Jun 2, 2021

View reviewed changes

mattklein123 requested changes Jun 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 2, 2021

address Matt's comments.

832b098

Signed-off-by: Renjie Tang <renjietang@google.com>

RenjieTang dismissed alyssawilk’s stale review via 832b098 June 2, 2021 22:18

repokitteh-read-only bot removed the waiting label Jun 2, 2021

mattklein123 reviewed Jun 2, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 2, 2021

add corner case test.

8b7047a

Signed-off-by: Renjie Tang <renjietang@google.com>

repokitteh-read-only bot removed the waiting label Jun 2, 2021

fix doc underline

0cc03f4

Signed-off-by: Renjie Tang <renjietang@google.com>

mattklein123 approved these changes Jun 3, 2021

View reviewed changes

mattklein123 merged commit 8e0fec1 into envoyproxy:main Jun 3, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

quic: add QUIC downstream connection close error stats. (envoyproxy#1…

290f241

…6584) Signed-off-by: Renjie Tang <renjietang@google.com>

		// TODO(renjietang): Currently these stats are only available in downstream. Wire it up to upstream
		// QUIC also.

Conversation

RenjieTang commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RenjieTang commented May 19, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

RenjieTang commented May 20, 2021

Uh oh!

jmarantz commented May 21, 2021

Uh oh!

RenjieTang commented May 21, 2021

Uh oh!

jmarantz 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!

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!

RenjieTang 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!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented May 21, 2021

Uh oh!

RenjieTang commented May 21, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

RenjieTang commented May 26, 2021

Uh oh!

repokitteh-read-only bot commented May 26, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented May 28, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

RenjieTang commented May 19, 2021 •

edited

Loading