socket: add SO_KEEPALIVE option for upstream connections. by JonathanO · Pull Request #3042 · envoyproxy/envoy

JonathanO · 2018-04-10T16:52:34Z

Description:
Adds TCP Keepalive support for upstream connections. This can be configured on the cluster manager level, and overridden on the cluster level.

Risk Level: Medium

Testing:
Unit tests have been added. It appears to run and work.

Docs Changes:
envoyproxy/data-plane-api#614

Fixes #3028

API Changes:
envoyproxy/data-plane-api#614

Adds support for configuring TCP Keepalives on upstream connections. Resolves envoyproxy#3028 Requires envoyproxy/data-plane-api#614 Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO · 2018-04-11T13:16:53Z

source/common/upstream/upstream_impl.cc

+      }
+    }
+    cluster_options->emplace_back(new Network::TcpKeepaliveOptionImpl(
+        PROTOBUF_GET_WRAPPED_OR_DEFAULT(cluster.tcpKeepaliveSettings(), keepalive_probes,


This is horrible and I shouldn't have done it this way. I'll move it out to a struct that'll be populated at construction time.

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

ggreenway · 2018-04-12T21:39:03Z

include/envoy/network/listen_socket.h

 typedef std::unique_ptr<ConnectionSocket> ConnectionSocketPtr;

+struct TcpKeepaliveConfig {
+  absl::optional<uint32_t> keepalive_probes_ = absl::nullopt;


No need for "= absl::nullopt;". The default constructor sets them properly.

ggreenway · 2018-04-12T22:06:32Z

include/envoy/upstream/cluster_manager.h

+   * @return const envoy::api::v2::UpstreamConnectionOptions& cluster manager wide connection
+   * options for new upstream connections.
+   */
+  virtual const envoy::api::v2::UpstreamConnectionOptions& connectionOptions() const PURE;


Given that configs are usually machine-generated, should we remove this (and the corresponding data-plane-api field) and just set this on each cluster?

This allows it to be set in the bootstrap config easily, and makes it's behaviour consistent with upstream_bind_options.

ggreenway · 2018-04-12T22:10:51Z

source/common/upstream/upstream_impl.cc

+                                        absl::optional<uint32_t>()),
+        PROTOBUF_GET_WRAPPED_OR_DEFAULT(connection_options.tcp_keepalive(), keepalive_interval,
+                                        absl::optional<uint32_t>())};
+  } else if (config.upstream_connection_options().has_tcp_keepalive()) {


These two cases can be refactored and collapsed into one, just operating on a different object.

ggreenway · 2018-04-12T22:16:51Z

source/common/upstream/upstream_impl.cc

    cluster_options = options;
  }
+  if (cluster.features() & ClusterInfo::Features::USE_TCP_KEEPALIVE) {
+    if (!cluster_options) {


Please clean up with duplicate logic from the FREEBIND case above. This is hard to follow.

ggreenway · 2018-04-12T22:20:45Z

source/common/upstream/upstream_impl.cc

  } else {
    cluster_options = options;
  }
+  if (cluster.features() & ClusterInfo::Features::USE_TCP_KEEPALIVE) {


Instead of adding a big to features(), can you just condition on whether cluster.tcpKeepaliveSettings() is set or not, and turn it into an optional?

I copied this pattern from the HTTP2 feature, I assumed there was maybe a performance reason for using the features flags rather than checking an optional field. If there's not, then I'll change it.

Looking at this further, I'm wondering if I should actually refactor to instantiate the TcpKeepaliveOptionImpl (and also UpstreamSocketOption for freebind) at ClusterInfoImpl construct time and store them both in a Options vector on ClusterInfo.
That removes them both from Features.
That should also greatly simplify the code in createConnection, since there's no need to have different cases for them (it's just a case of merging options vectors together is necessary.)
This should be safe as neither Option implementation stores any state that differs between connections to the same cluster.

What do you think?

My best guess for why this is the way it is: the features flags came befofe the current iteration of socket-option-setters.

I think your suggested refactoring sounds great, and should be a nice cleanup. @htuch do you know any more history on this, or any objections to this cleanup?

I've made an attempt at the refactor (based off this branch) in this commit JonathanO@bf4bf78 which I think looks a bit cleaner.

I must admit that I'm (re)learning modern C++ by trial and error, and I have no idea if the shared pointer usage in that commit is right or even necessary.
If it looks sane then I'll merge it into this PR.

Yeah, that looks better to me. I'll add a few comments when you merge that into this PR, but the basic structure seems right.

ggreenway · 2018-04-12T22:22:34Z

test/common/network/tcp_keepalive_option_impl_test.cc

+// We fail to set the option when the underlying setsockopt syscall fails.
+TEST_F(TcpKeepaliveOptionImplTest, SetOptionTcpKeepaliveFailure) {
+  TcpKeepaliveOptionImpl socket_option{{}};
+#ifdef SO_KEEPALIVE


if (ENVOY_SOCKET_SO_KEEPALIVE.has_value())

ggreenway · 2018-04-12T22:23:34Z

source/common/network/tcp_keepalive_option_impl.cc

+}
+
+bool TcpKeepaliveOptionImpl::setTcpKeepalive(Socket& socket,
+                                             const absl::optional<uint32_t>& keepalive_probes,


These 3 params can be passed by value instead of by const&

ggreenway · 2018-04-12T22:28:40Z

source/common/network/tcp_keepalive_option_impl.cc

+    ENVOY_LOG(warn, "Setting SO_KEEPALIVE on socket failed: {}", strerror(error));
+    return false;
+  }
+  if (keepalive_probes.has_value()) {


Please factor out a function for the pattern "if option doesn't exist then return false; set the option; check the error" and call it 4 times.

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Removes options on cluster manager, so no need for handing overriding of the socket options anymore. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

ggreenway

This is turning out really nice. Thanks for cleaning up the freebind features thing.

ggreenway · 2018-04-17T15:46:46Z

source/common/network/tcp_keepalive_option_impl.cc

+                                             absl::optional<uint32_t> keepalive_time,
+                                             absl::optional<uint32_t> keepalive_interval) {
+  int error;
+  error = setSocketOption(socket, SOL_SOCKET, ENVOY_SOCKET_SO_KEEPALIVE, 1);


Q: does there need to be a way to configure it to explicitly disable the socket option, ie set it to a value of zero? I think it would only be needed if there's a way to make the default (at the OS level) enabled. Is that a thing we need to do?

Under Linux there's no way to turn it on at the OS level, only tweak the parameters. It appears that under FreeBSD and OSX it's possible to turn it on for everyone with "net.inet.tcp.always_keepalive", but I don't know if it's possible to explicitly disable it using SO_KEEPALIVE=0 in that case.

ggreenway · 2018-04-17T15:48:23Z

source/common/network/tcp_keepalive_option_impl.cc

+int TcpKeepaliveOptionImpl::setSocketOption(Socket& socket, int level,
+                                            Network::SocketOptionName optname, uint32_t value) {
+  if (!optname.has_value()) {
+    return ENOTSUP;


This should return -1, and set errno to ENOTSUP

ggreenway · 2018-04-17T15:50:17Z

source/common/network/tcp_keepalive_option_impl.cc

+    return ENOTSUP;
+  }
+  auto& os_syscalls = Api::OsSysCallsSingleton::get();
+  return os_syscalls.setsockopt(socket.fd(), level, optname.value(), &value, sizeof(value));


These options should all be of type/size int, not uint32_t. See http://man7.org/linux/man-pages/man7/tcp.7.html

ggreenway · 2018-04-17T15:50:42Z

source/common/network/tcp_keepalive_option_impl.cc

+}
+
+} // namespace Network
+} // namespace Envoy


Please add a newline at the end of the file

ggreenway · 2018-04-17T15:57:15Z

api/envoy/api/v2/core/address.proto

  bool ipv4_compat = 6;
 }

+message TcpKeepalive {


Please document the macos behavior. It looks like you added support for the right socket option, but please document that the other two must be unset in the config on macos.

I swear I replied to this, but the comment seems to be missing now.
The reason for the single special case is that MacOS uses a different name for that one constant (TCP_KEEPIDLE vs TCP_KEEPALIVE.) The other constants all have the same name as for Linux. All of these settings are valid for both Linux and MacOS. I've added a comment to that effect in the header where they're defined.

ggreenway · 2018-04-17T16:05:22Z

source/common/upstream/upstream_impl.cc

+    cluster_options->emplace_back(
+        new Network::TcpKeepaliveOptionImpl(parseTcpKeepaliveConfig(config)));
+  }
+  return cluster_options;


if cluster_options is empty, return nullptr?

ggreenway · 2018-04-17T16:06:30Z

source/common/upstream/upstream_impl.cc

+  // Cluster IP_FREEBIND settings, when set, will override the cluster manager wide settings.
+  if ((bind_config.freebind().value() && !config.upstream_bind_config().has_freebind()) ||
+      config.upstream_bind_config().freebind().value()) {
+    cluster_options->emplace_back(new UpstreamSocketOption(true));


std::make_shared

ggreenway · 2018-04-17T16:06:39Z

source/common/upstream/upstream_impl.cc

+  }
+  if (config.upstream_connection_options().has_tcp_keepalive()) {
+    cluster_options->emplace_back(
+        new Network::TcpKeepaliveOptionImpl(parseTcpKeepaliveConfig(config)));


std::make_shared

ggreenway · 2018-04-17T16:08:29Z

source/common/upstream/upstream_impl.cc

-  if (cluster.features() & ClusterInfo::Features::FREEBIND) {
-    cluster_options = std::make_shared<Network::ConnectionSocket::Options>();
+  Network::ConnectionSocket::OptionsSharedPtr connection_options;
+  if (cluster.clusterSocketOptions() && !cluster.clusterSocketOptions().get()->empty()) {


if (cluster.clusterSocketOptions() != nullptr && !cluster.clusterSocketOptions()->empty()) {

Can be simplified to just the nullptr check if parseClusterSocketOptions() returns nullptr if the options are empty, as you suggest.

ggreenway · 2018-04-17T16:11:23Z

source/common/upstream/upstream_impl.cc

-      *cluster_options = *options;
+      connection_options = std::make_shared<Network::ConnectionSocket::Options>();
+      *connection_options = *options;
+      copy(cluster.clusterSocketOptions().get()->begin(),


std::copy and std::back_inserter?

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

htuch · 2018-04-18T14:09:42Z

include/envoy/network/listen_socket.h


 typedef std::unique_ptr<ConnectionSocket> ConnectionSocketPtr;

+struct TcpKeepaliveConfig {


I think this can be moved closer to the _impl.cc, since it's an implementation detail essentially and not needed by any interfaces in include/envoy.

htuch · 2018-04-18T14:10:14Z

include/envoy/network/listen_socket.h


+struct TcpKeepaliveConfig {
+  absl::optional<uint32_t> keepalive_probes_;
+  absl::optional<uint32_t> keepalive_time_;


Comments would be good, in particular providing the units of time being used.

htuch · 2018-04-18T14:20:58Z

source/common/network/tcp_keepalive_option_impl.cc

+    return -1;
+  }
+  auto& os_syscalls = Api::OsSysCallsSingleton::get();
+  return os_syscalls.setsockopt(socket.fd(), level, optname.value(), &value, sizeof(value));


I feel like we're redoing https://github.com/envoyproxy/envoy/blob/master/source/common/network/socket_option_impl.h here. I realize these are TCP options, there is also #2793 where I've asked to add in some equivalent support for TCP options. Do you think it's reasonable to come up with some refactoring to avoid redoing how socket options are set, platform features are handled and the various branches tested?

I'm going to change SocketOptionName to be absl::optional<std::pair<int, int>>, so it can contain the "level" as well as "optname." That should make it possible to clean up the socket option code a bit and maybe make a common Socket::Option implementation, which takes a SocketOptionName and value as constructor args, possible. That might then allow TCP Keepalive, and other simple options, to be implemented with as a factory that produces a vector of options.

SocketOptionImpl has special handling for checking if the socket it's being applied to is IP, and setting different options for IPV6 vs 4. I think that's going to have to remain slightly special (although it might be able to use the generic implementation under the hood.)

Awesome. @bmetzdorf for visibility, so that you folks don't collide on this. I could merge #2793 as is if that works for you both.

Yeah, that works for me.

SGTM, I can address the open aesthetic items from #2793 in another PR.

OK, will merge.

#2793 merged.

…-option

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

…-option

JonathanO · 2018-04-24T16:21:00Z

This should be ready for review again now. I've merged the fast open changes from master, and completed a refactor to hopefully clean up all of the socket option handling.

htuch

I like the new approach overall, very clean. I do have some reservations on SocketOptionsImpl. Also @bmetzdorf for thoughts on the TCP socket options, I know he is working on this right now in #3157

htuch · 2018-04-24T17:54:19Z

source/common/network/addr_family_aware_socket_impl.cc

@@ -0,0 +1,58 @@
+#include "common/network/addr_family_aware_socket_impl.h"


This file is missing a bunch of code coverage.

htuch · 2018-04-24T17:54:27Z

source/common/network/socket_option_factory.cc

@@ -0,0 +1,59 @@
+#include "common/network/socket_option_factory.h"


This file is missing a bunch of code coverage.

htuch · 2018-04-24T17:58:05Z

source/common/network/socket_option_impl.h

+
+  // Socket::Option
+  bool setOption(Socket& socket, Socket::SocketState state) const override {
+    bool good = true;


Nit: prefer renaming this to be something more descriptive, e.g. all_socket_options_applied. Coming to think of it, I'm wondering exactly what semantics we want here.. do we want to abort early and just return false in the inner loop rather than continue to apply options?

I think you're right, it should abort early since all other loops that call setOption() already abort early. I can't think of a reason not to, and I don't think there was any specific reason I chose this implementation at the time.

htuch · 2018-04-24T17:59:23Z

source/common/network/socket_option_impl.h

+  const int value_;
+};
+
+class SocketOptionsImpl : public Socket::Option, Logger::Loggable<Logger::Id::connection> {


Why do we need this given that Socket already has a collection of options? Doesn't this then become a collection of options for a collection of options?

This was an attempt to allow the socket option factory methods that want to set multiple options (e.g. the TCP keepalive one) to still return a single Socket::Option that the caller would then do whatever they needed to in order to add it to their own list.

The alternatives seemed to be to return a Socket::Options and require the caller to copy the contents of that into their own list (seemed a bit ugly), or for the factory to be provided with an OptionsSharedPtr for the factory to append to (but that doesn't quite mesh with Listener's addListenSocketOption implementation.)

Another alternative would be for me to maybe replace the Socket::Options typedef with an implementation like this one, which would avoid us needing two different types of collection of Options, but would still leave us with nested collections of options (just both levels of the same type.)

I'm tending towards the last idea, but do you have any preference (or a better alternative?)

The two ideas I like are the last one (always use a special collection object) or for the factory to always return a vector/list (Socket::Options). I lean towards always using Socket::Options because then there isn't an issue with weird nesting.

With either of these approaches, I think eventually it would be better for each socketoption to have it's own object. You don't have to make that change as part of this PR if you don't want, but having 1 option per object, and a consistent way to dealing with a collection/set of these objects, would make this a bit cleaner.

Yeah, I also like:

One socket option per object.

Factory returns always Socket::Options

OK, I'll go that path, and for this PR I'll add a bunch of shared utility functions for managing these collections to avoid duplicating code in listener and upstream (rather than adding a new collection type with methods for the needed operations.)

I've just pushed the change to get rid of SocketOptionsImpl. It would definitely look even cleaner with a new collection type, rather than static utility methods I added on Socket, but I think maybe this is already a sufficient improvement? I'm still missing test coverage as @htuch pointed out, which I'll fix tomorrow.

Added utility methods to Network::Socket instead and updated listener and upstream to use them. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO · 2018-04-25T17:16:15Z

include/envoy/network/listen_socket.h

 #include "envoy/common/pure.h"
 #include "envoy/network/address.h"

+#include "absl/types/optional.h"


Unneeded include

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

…-option

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

htuch

LGTM, this has turned out quite nicely. @ggreenway any further comments?

ggreenway · 2018-04-27T21:53:30Z

@JonathanO You have merge conflicts to resolve.

ggreenway

This looks great! Thanks for refactoring this.

I think this is ready once the merge conflicts are resolved.

…-option

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

htuch

Thanks @JonathanO, this is a really great improvement on what we had.

…#3042) Adds TCP Keepalive support for upstream connections. This can be configured on the cluster manager level, and overridden on the cluster level. Risk Level: Medium Testing: Unit tests have been added. It appears to run and work. Docs Changes: envoyproxy/data-plane-api#614 Fixes envoyproxy#3028 API Changes: envoyproxy/data-plane-api#614 Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com> Signed-off-by: Rama <rama.rao@salesforce.com>

JonathanO added 6 commits April 10, 2018 14:24

Add SO_KEEPALIVE option for upstream connections.

0d71615

Adds support for configuring TCP Keepalives on upstream connections. Resolves envoyproxy#3028 Requires envoyproxy/data-plane-api#614 Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Remove incorrect import

7017ffc

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Hack data-plane-api version.

bd79a31

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Correct commit hash for data-plane-api

c5ddd1b

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Fix MacOS constant naming conflict

61537b7

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Updated for data-plane-api changes.

fad89fd

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO force-pushed the add-tcp-keepalive-option branch from 6a6ae7c to fad89fd Compare April 11, 2018 13:15

JonathanO commented Apr 11, 2018

View reviewed changes

JonathanO added 3 commits April 11, 2018 15:08

Parse tcp keepalive settings into a struct at construct time.

cb216a1

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Use new keepalive config struct in the option.

77aee92

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Fix format build failure.

23acd5c

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

dnoe assigned ggreenway and htuch Apr 12, 2018

ggreenway requested changes Apr 12, 2018

View reviewed changes

JonathanO added 2 commits April 13, 2018 15:34

Code review fixes.

894a6d4

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Refactoring to avoid use of Features.

c813195

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO force-pushed the add-tcp-keepalive-option branch from cf72a27 to c813195 Compare April 13, 2018 19:45

Updated to new proposed data-plane-api version

8a88054

Removes options on cluster manager, so no need for handing overriding of the socket options anymore. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO force-pushed the add-tcp-keepalive-option branch from 90bbe0e to 8a88054 Compare April 13, 2018 20:31

Merge branch 'master' into add-tcp-keepalive-option

313743c

htuch mentioned this pull request Apr 16, 2018

Add SO_KEEPALIVE option for upstream connections. envoyproxy/data-plane-api#614

Closed

Moved API changes from data-plane-api repo.

937a793

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO force-pushed the add-tcp-keepalive-option branch from 339fd57 to 937a793 Compare April 16, 2018 20:26

ggreenway requested changes Apr 17, 2018

View reviewed changes

Code review changes.

15d52ce

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO force-pushed the add-tcp-keepalive-option branch from f5fdca6 to 15d52ce Compare April 18, 2018 13:56

htuch suggested changes Apr 18, 2018

View reviewed changes

JonathanO added 3 commits April 19, 2018 21:19

Merge remote-tracking branch 'upstream/master' into add-tcp-keepalive…

43d117d

…-option

Include "level" in SocketOptionName

238894e

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Generic socket options holder and factories.

d85f3dd

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO added 2 commits April 24, 2018 16:22

Code review update and style fixes.

0c81bbb

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Merge remote-tracking branch 'upstream/master' into add-tcp-keepalive…

3d15a3d

…-option

htuch suggested changes Apr 24, 2018

View reviewed changes

Removed SocketOptionsImpl

e275246

Added utility methods to Network::Socket instead and updated listener and upstream to use them. Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO force-pushed the add-tcp-keepalive-option branch from 2e84117 to e275246 Compare April 25, 2018 17:12

JonathanO commented Apr 25, 2018

View reviewed changes

bmetzdorf mentioned this pull request Apr 25, 2018

connection: Add TCP_FASTOPEN listener option (continuation) #3157

Merged

JonathanO added 4 commits April 26, 2018 14:43

Renamed misnamed class, added test coverage.

c0f4cdc

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Merge remote-tracking branch 'upstream/master' into add-tcp-keepalive…

c1246bf

…-option

Code style fixes.

99ac729

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

Fix BUILD file format.

588d234

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

htuch previously approved these changes Apr 27, 2018

View reviewed changes

ggreenway previously approved these changes Apr 27, 2018

View reviewed changes

JonathanO added 2 commits April 28, 2018 15:54

Merge remote-tracking branch 'upstream/master' into add-tcp-keepalive…

7a37ab7

…-option

Added test case for all Keepalive options

4c40a6b

Signed-off-by: Jonathan Oddy <jonathan.oddy@transferwise.com>

JonathanO dismissed stale reviews from ggreenway and htuch via 4c40a6b April 28, 2018 15:01

htuch approved these changes Apr 29, 2018

View reviewed changes

htuch merged commit dd953f9 into envoyproxy:master Apr 29, 2018

JonathanO deleted the add-tcp-keepalive-option branch April 30, 2018 10:18


		typedef std::unique_ptr<ConnectionSocket> ConnectionSocketPtr;

		struct TcpKeepaliveConfig {

		@@ -0,0 +1,58 @@
		#include "common/network/addr_family_aware_socket_impl.h"

		@@ -0,0 +1,59 @@
		#include "common/network/socket_option_factory.h"

Conversation

JonathanO commented Apr 10, 2018

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!

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!

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

JonathanO Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

JonathanO Apr 18, 2018 •

edited

Loading