Kubernetes watch implementation. #45

igorpeshansky · 2017-12-19T00:46:44Z

Reviewers, please let me know if this needs to be squashed.

igorpeshansky · 2017-12-19T00:47:29Z

@supriyagarg, would be great if you could review as well.

bmoyles0117 · 2017-12-19T15:10:54Z

src/kubernetes.cc

+          std::unique_lock<std::mutex>&& completion, bool verbose)
+      : completion_(std::move(completion)), callback_(callback),
+        remaining_bytes_(0), verbose_(verbose) {}
+  ~Watcher() {}  // Unlocks the completion_ lock.


Is unlocking the completion lock inherent, or do you still need to add that?

I think you meant s/inherent/implicit/. Yes, unlocking is implicit, through the semantics of std::unique_lock.

bmoyles0117 · 2017-12-19T15:18:27Z

src/kubernetes.cc

+      LOG(INFO) << "Waiting for completion";
+    }
+    std::lock_guard<std::mutex> await_completion(completion_mutex);
+    if (config_.VerboseLogging()) {


Nit: the varying conditionals for exporting logs begs for a level-based log filter.

We already have a level-based log filter. I think you mean something like a semantic module-based log filter. I have an item on my list to rework logging and make it sane, but that won't happen any time soon.

bmoyles0117 · 2017-12-19T15:20:10Z

src/kubernetes.cc

+    if (config_.VerboseLogging()) {
+      LOG(INFO) << "Waiting for completion";
+    }
+    std::lock_guard<std::mutex> await_completion(completion_mutex);


We should sync up with the Kubernetes folks to see what conditions a timeout would occur when watching resources. My concern is we might face a deadlock situation here should the connection be left hanging.

This mutex is unlocked when the watch thread sees an EOF from the server or encounters an error.

Is an EOF enough to stop this thread? If so, we should definitely exit and trigger a restart.

An EOF means that the master terminated the watch connection. I would treat this as an exceptional situation.

bmoyles0117 · 2017-12-19T15:21:43Z

src/kubernetes.cc

+      const std::string& ns = KubernetesNamespace();
+      // TODO: This is unreliable, see
+      // https://github.com/kubernetes/kubernetes/issues/52162.
+      const std::string pod_name = boost::asio::ip::host_name();


Should we also add a conditional for an environment variable, that way we have at least one more option before going the unreliable route?

Which environment variable? Do you mean $HOSTNAME? Or something set via the downward API?

Something via the downward api. With enough documentation it will likely be more reliable than the hostname.

I'm happy to entertain various options here, but let's open a bug and track this separately.

bmoyles0117 · 2017-12-19T15:22:59Z

src/kubernetes.cc

+            "/namespaces/" + ns + "/pods/" + pod_name);
+        const json::Object* pod = pod_response->As<json::Object>();
+        const json::Object* spec = pod->Get<json::Object>("spec");
+        current_node_ = spec->Get<json::String>("nodeName");


In theory, if hostname ended up pointing at an unscheduled pod (through user error), it's possible for nodeName to not be set. Crazy edge-case, but possible.

This code examines the pod that the metadata agent itself is running in. Since the binary is executing, I think it's safe to assume the pod is scheduled... :-)

This concern is mostly vocalized due to the fact that using hostname is not reliable, and could lead to some strange behavior in the first place. If you feel that that's fine for now, this SGTM.

bmoyles0117 · 2017-12-19T15:25:09Z

src/kubernetes.cc

+  LOG(ERROR) << "Watch type: " << type << " object: " << *pod;
+  if (type == "MODIFIED" || type == "ADDED") {
+    std::vector<MetadataUpdater::ResourceMetadata> result_vector =
+        GetPodAndContainerMetadata(pod, collected_at);


Why is it this callback's responsibility to generate the collected_at time? Could we move it to within the function?

It's not at all certain that the callback will be executed immediately after the watch notification. Threads get delayed, etc. I'd rather have the timestamp as close to the actual change time as possible.

I'm confused, at what point between where collected_at and GetPodContainerMetadata being invoked, is a thread being spawned? It seems that all of this code IS the thread in question.

The thread can get preempted between the time the watch notification is received and the time the response is parsed. Relegating the computation of collected_at to the callback would increase the chances of that happening.

bmoyles0117 · 2017-12-19T15:27:56Z

src/kubernetes.cc

+  } catch (const KubernetesReader::QueryException& e) {
+    // Already logged.
+  }
+  LOG(INFO) << "Watch thread (pods) exiting";


Where do we have a looping mechanism to ensure that we re-establish a connection should our connection to the master get cut?

Nowhere yet. Let's open a bug to improve the robustness of this code (probably need to guard it by a config option as well).

Will do, SGTM.

bmoyles0117 · 2017-12-19T15:28:26Z

src/kubernetes.cc

+  LOG(ERROR) << "Watch type: " << type << " object: " << *node;
+  if (type == "MODIFIED" || type == "ADDED") {
+    std::vector<MetadataUpdater::ResourceMetadata> result_vector;
+    result_vector.emplace_back(GetNodeMetadata(node->Clone(), collected_at));


Same concern here for collected_at.

Same response.

bmoyles0117 · 2017-12-19T15:29:57Z

src/kubernetes.cc

+
+  try {
+    // TODO: There seems to be a Kubernetes API bug with watch=true.
+    WatchMaster(std::string(kKubernetesEndpointPath) + "/watch/nodes/"


We should resolve this before merging, or use /watch/ for pods as well if possible.

Hmm, this works just fine for pods. Frankly, I don't see it as a merge blocker. We should absolutely open a bug and investigate at leisure.

Will do, SGTM.

igorpeshansky

Responded. PTAL.

igorpeshansky · 2017-12-19T16:48:31Z

src/kubernetes.cc

+          std::unique_lock<std::mutex>&& completion, bool verbose)
+      : completion_(std::move(completion)), callback_(callback),
+        remaining_bytes_(0), verbose_(verbose) {}
+  ~Watcher() {}  // Unlocks the completion_ lock.


I think you meant s/inherent/implicit/. Yes, unlocking is implicit, through the semantics of std::unique_lock.

igorpeshansky · 2017-12-19T16:50:38Z

src/kubernetes.cc

+    if (config_.VerboseLogging()) {
+      LOG(INFO) << "Waiting for completion";
+    }
+    std::lock_guard<std::mutex> await_completion(completion_mutex);


This mutex is unlocked when the watch thread sees an EOF from the server or encounters an error.

igorpeshansky · 2017-12-19T16:51:48Z

src/kubernetes.cc

+      LOG(INFO) << "Waiting for completion";
+    }
+    std::lock_guard<std::mutex> await_completion(completion_mutex);
+    if (config_.VerboseLogging()) {


We already have a level-based log filter. I think you mean something like a semantic module-based log filter. I have an item on my list to rework logging and make it sane, but that won't happen any time soon.

igorpeshansky · 2017-12-19T16:53:57Z

src/kubernetes.cc

+      const std::string& ns = KubernetesNamespace();
+      // TODO: This is unreliable, see
+      // https://github.com/kubernetes/kubernetes/issues/52162.
+      const std::string pod_name = boost::asio::ip::host_name();


Which environment variable? Do you mean $HOSTNAME? Or something set via the downward API?

igorpeshansky · 2017-12-19T16:54:58Z

src/kubernetes.cc

+            "/namespaces/" + ns + "/pods/" + pod_name);
+        const json::Object* pod = pod_response->As<json::Object>();
+        const json::Object* spec = pod->Get<json::Object>("spec");
+        current_node_ = spec->Get<json::String>("nodeName");


This code examines the pod that the metadata agent itself is running in. Since the binary is executing, I think it's safe to assume the pod is scheduled... :-)

igorpeshansky · 2017-12-19T16:56:35Z

src/kubernetes.cc

+  LOG(ERROR) << "Watch type: " << type << " object: " << *pod;
+  if (type == "MODIFIED" || type == "ADDED") {
+    std::vector<MetadataUpdater::ResourceMetadata> result_vector =
+        GetPodAndContainerMetadata(pod, collected_at);


It's not at all certain that the callback will be executed immediately after the watch notification. Threads get delayed, etc. I'd rather have the timestamp as close to the actual change time as possible.

igorpeshansky · 2017-12-19T16:57:34Z

src/kubernetes.cc

+  } catch (const KubernetesReader::QueryException& e) {
+    // Already logged.
+  }
+  LOG(INFO) << "Watch thread (pods) exiting";


Nowhere yet. Let's open a bug to improve the robustness of this code (probably need to guard it by a config option as well).

igorpeshansky · 2017-12-19T16:57:52Z

src/kubernetes.cc

+  LOG(ERROR) << "Watch type: " << type << " object: " << *node;
+  if (type == "MODIFIED" || type == "ADDED") {
+    std::vector<MetadataUpdater::ResourceMetadata> result_vector;
+    result_vector.emplace_back(GetNodeMetadata(node->Clone(), collected_at));


Same response.

igorpeshansky · 2017-12-19T16:58:43Z

src/kubernetes.cc

+
+  try {
+    // TODO: There seems to be a Kubernetes API bug with watch=true.
+    WatchMaster(std::string(kKubernetesEndpointPath) + "/watch/nodes/"


Hmm, this works just fine for pods. Frankly, I don't see it as a merge blocker. We should absolutely open a bug and investigate at leisure.

igorpeshansky

Responded. PTAL.

igorpeshansky · 2017-12-19T19:41:25Z

src/kubernetes.cc

+    if (config_.VerboseLogging()) {
+      LOG(INFO) << "Waiting for completion";
+    }
+    std::lock_guard<std::mutex> await_completion(completion_mutex);


An EOF means that the master terminated the watch connection. I would treat this as an exceptional situation.

igorpeshansky · 2017-12-19T19:43:34Z

src/kubernetes.cc

+      const std::string& ns = KubernetesNamespace();
+      // TODO: This is unreliable, see
+      // https://github.com/kubernetes/kubernetes/issues/52162.
+      const std::string pod_name = boost::asio::ip::host_name();


I'm happy to entertain various options here, but let's open a bug and track this separately.

igorpeshansky · 2017-12-19T19:44:59Z

src/kubernetes.cc

+  LOG(ERROR) << "Watch type: " << type << " object: " << *pod;
+  if (type == "MODIFIED" || type == "ADDED") {
+    std::vector<MetadataUpdater::ResourceMetadata> result_vector =
+        GetPodAndContainerMetadata(pod, collected_at);


The thread can get preempted between the time the watch notification is received and the time the response is parsed. Relegating the computation of collected_at to the callback would increase the chances of that happening.

bmoyles0117

LGTM, last thing I would request is to remove all of the disabled comments. 🥇

qingling128 · 2017-12-19T23:03:46Z

src/kubernetes.cc

+    const std::string crlf("\r\n");
+    auto begin = std::begin(range);
+    auto end = std::end(range);
+    const size_t available = std::distance(begin, end);


Not sure if it's just me, but the naming of available and remaining_bytes threw me off a bit. I thought available is the available size in this current chunk while remaining_bytes is the total remaining bytes that we still need to read. After a closer look, I think it's in fact the other way around.

available is the available bytes in the current notification, which may include the remainder of this chunk and the start of the next one. Suggestions welcome.

Maybe add a comment?

// The available bytes in the current notification, which may include the remainder of this chunk and the start of the next one.

Done. Also renamed remaining_bytes_ to remaining_chunk_bytes_.

qingling128 · 2017-12-19T23:24:55Z

src/kubernetes.cc

+        CompleteChunk();
+
+        // Process the next batch.
+        while (remaining_bytes_ == 0 && std::begin(pos) != std::end(pos)) {


Is this while loop required only because we might run into blank lines? It took me a while to digest why there is a loop here while ReadNextChunk and CompleteChunk don't have loops (because the whole thing will be triggered repeatedly).

Yes, this is a callback that collects data. It may get partial data, or it may get data that's on the boundary between two chunks.
The while loop is because we may get dud (empty) chunks. It basically says: while there's still data available and we haven't started a new chunk, try to start one. Eventually it either runs out of data (std::begin(pos) == std::end(pos)) or starts a non-empty chunk (remaining_bytes_ != 0).
Now that you point it out, there's a way to split this out into a streaming callback that only handles the chunked encoding and then invokes another callback on each full chunk. The former could even be contributed upstream. Since this won't change the basic functionality, I would defer this refactoring until later. We should open a bug, though.

Makes sense. I created b/70889561 for the potential refactoring.

qingling128 · 2017-12-19T23:29:31Z

src/kubernetes.cc

+    }
+    std::string line(begin, iter);
+    iter = std::next(iter, crlf.size());
+    if (line.empty()) {


Why don't we return early when the line is empty? Something like:

const std::string crlf("\r\n"); auto begin = std::begin(range); auto end = std::end(range); auto iter = std::search(begin, end, crlf.begin(), crlf.end()); if (iter == begin) { // Blank lines are fine, just skip them. #ifdef VERBOSE LOG(DEBUG) << "Skipping blank line within chunked encoding;" << " remaining data '" << std::string(iter, end) << "'"; #endif return boost::iterator_range<const char*>(begin, end); } else if (iter == end) { LOG(ERROR) << "Invalid chunked encoding: '" << std::string(begin, end) << "'"; return boost::iterator_range<const char*>(begin, end); } std::string line(begin, iter); iter = std::next(iter, crlf.size()); //#ifdef VERBOSE // LOG(DEBUG) << "Line: '" << line << "'"; //#endif std::stringstream stream(line); stream >> std::hex >> remaining_bytes_; return boost::iterator_range<const char*>(iter, end);

Your proposed code is not quite equivalent. You're correct that line.empty() is equivalent to iter == begin (before the iter reassignment). However, in the case of empty line we return boost::iterator_range<const char*>(iter, end) after the reassignment, so we're skipping the crlf sequence.
I could do it the way you're proposing, but then I would have to replicate the code that skips crlf. WDYT?

Oops, right. I was missing the iter = std::next(iter, crlf.size()); part. I'd still vote for keeping the edge cases (invalid chunk and blank line) together though.

Yes, you're right, it does read better. Changed.

igorpeshansky

Responded. PTAL.

igorpeshansky · 2017-12-19T23:46:08Z

src/kubernetes.cc

+        CompleteChunk();
+
+        // Process the next batch.
+        while (remaining_bytes_ == 0 && std::begin(pos) != std::end(pos)) {


Yes, this is a callback that collects data. It may get partial data, or it may get data that's on the boundary between two chunks.
The while loop is because we may get dud (empty) chunks. It basically says: while there's still data available and we haven't started a new chunk, try to start one. Eventually it either runs out of data (std::begin(pos) == std::end(pos)) or starts a non-empty chunk (remaining_bytes_ != 0).
Now that you point it out, there's a way to split this out into a streaming callback that only handles the chunked encoding and then invokes another callback on each full chunk. The former could even be contributed upstream. Since this won't change the basic functionality, I would defer this refactoring until later. We should open a bug, though.

igorpeshansky · 2017-12-19T23:59:03Z

src/kubernetes.cc

+    }
+    std::string line(begin, iter);
+    iter = std::next(iter, crlf.size());
+    if (line.empty()) {


Your proposed code is not quite equivalent. You're correct that line.empty() is equivalent to iter == begin (before the iter reassignment). However, in the case of empty line we return boost::iterator_range<const char*>(iter, end) after the reassignment, so we're skipping the crlf sequence.
I could do it the way you're proposing, but then I would have to replicate the code that skips crlf. WDYT?

igorpeshansky · 2017-12-20T00:00:26Z

src/kubernetes.cc

+    const std::string crlf("\r\n");
+    auto begin = std::begin(range);
+    auto end = std::end(range);
+    const size_t available = std::distance(begin, end);


available is the available bytes in the current notification, which may include the remainder of this chunk and the start of the next one. Suggestions welcome.

qingling128 · 2017-12-20T18:06:02Z

src/kubernetes.cc

+    }
+    std::string line(begin, iter);
+    iter = std::next(iter, crlf.size());
+    if (line.empty()) {


Oops, right. I was missing the iter = std::next(iter, crlf.size()); part. I'd still vote for keeping the edge cases (invalid chunk and blank line) together though.

qingling128 · 2017-12-20T18:06:43Z

src/kubernetes.cc

+        CompleteChunk();
+
+        // Process the next batch.
+        while (remaining_bytes_ == 0 && std::begin(pos) != std::end(pos)) {


Makes sense. I created b/70889561 for the potential refactoring.

qingling128 · 2017-12-20T18:07:53Z

src/kubernetes.cc

+    const std::string crlf("\r\n");
+    auto begin = std::begin(range);
+    auto end = std::end(range);
+    const size_t available = std::distance(begin, end);


Maybe add a comment?

// The available bytes in the current notification, which may include the remainder of this chunk and the start of the next one.

igorpeshansky

Addressed feedback. PTAL.

igorpeshansky · 2017-12-20T19:08:58Z

src/kubernetes.cc

+    const std::string crlf("\r\n");
+    auto begin = std::begin(range);
+    auto end = std::end(range);
+    const size_t available = std::distance(begin, end);


Done. Also renamed remaining_bytes_ to remaining_chunk_bytes_.

igorpeshansky · 2017-12-20T19:34:36Z

src/kubernetes.cc

+    }
+    std::string line(begin, iter);
+    iter = std::next(iter, crlf.size());
+    if (line.empty()) {


Yes, you're right, it does read better. Changed.

qingling128

LGTM.

qingling128 · 2017-12-20T20:12:59Z

Please address the conflicts though.

supriyagarg · 2017-12-20T22:07:58Z

src/kubernetes.cc

+  json::value node_raw_metadata = json::object({
+    {"blobs", json::object({
+      {"association", json::object({
+        {"version", json::string(kRawContentVersion)},


What version does kRawContentVersion refer to?

It's an arbitrary constant we (jointly with the Resource Metadata API) picked to represent the version of the envelope object schema.

supriyagarg · 2017-12-20T22:40:13Z

src/kubernetes.cc

-        std::string(kKubernetesEndpointPath) + "/nodes/" + node_name);
-    Timestamp collected_at = std::chrono::system_clock::now();
+  // TODO: find is_deleted.
+  //const json::Object* status = pod->Get<json::Object>("status");


Drop the commented line?

Also, what does the TODO mean? Should it be "compute" is_deleted?

The commented line is a starting point for finding is_deleted (if it's anywhere, it's in the pod status). The point here is that we don't really know how to compute is_deleted, so this is a compromise between define is_deleted and compute is_deleted. :-)
Also, this seems to be an unrelated change from the earlier refactoring that accidentally cropped up in this PR. Let me rebase the commits off master.

supriyagarg · 2017-12-20T22:42:49Z

src/kubernetes.cc

+          docker_prefix_end);
+  // TODO: find is_deleted.
+  //const json::Object* state = container->Get<json::Object>("state");
+  bool is_deleted = false;


igorpeshansky

Responded to your comments. PTAL.

igorpeshansky · 2017-12-20T23:09:04Z

src/kubernetes.cc

+  json::value node_raw_metadata = json::object({
+    {"blobs", json::object({
+      {"association", json::object({
+        {"version", json::string(kRawContentVersion)},


It's an arbitrary constant we (jointly with the Resource Metadata API) picked to represent the version of the envelope object schema.

igorpeshansky · 2017-12-20T23:10:35Z

src/kubernetes.cc

-        std::string(kKubernetesEndpointPath) + "/nodes/" + node_name);
-    Timestamp collected_at = std::chrono::system_clock::now();
+  // TODO: find is_deleted.
+  //const json::Object* status = pod->Get<json::Object>("status");


The commented line is a starting point for finding is_deleted (if it's anywhere, it's in the pod status). The point here is that we don't really know how to compute is_deleted, so this is a compromise between define is_deleted and compute is_deleted. :-)
Also, this seems to be an unrelated change from the earlier refactoring that accidentally cropped up in this PR. Let me rebase the commits off master.

igorpeshansky · 2017-12-20T23:13:56Z

src/kubernetes.cc

+          docker_prefix_end);
+  // TODO: find is_deleted.
+  //const json::Object* state = container->Get<json::Object>("state");
+  bool is_deleted = false;


supriyagarg

Ah - this PR looks much shorter now :)

igorpeshansky · 2017-12-20T23:30:11Z

Yeah, rebasing does help. Sorry about the mess.

igorpeshansky requested review from bmoyles0117 and qingling128 December 19, 2017 00:46

bmoyles0117 reviewed Dec 19, 2017

View reviewed changes

igorpeshansky commented Dec 19, 2017

View reviewed changes

bmoyles0117 approved these changes Dec 19, 2017

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch from 86b5b28 to 9f60e88 Compare December 19, 2017 21:34

igorpeshansky force-pushed the igorp-kubernetes-watch branch from 610b9ce to 1448f21 Compare December 19, 2017 21:34

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch from 9f60e88 to bad0e2d Compare December 19, 2017 21:36

igorpeshansky force-pushed the igorp-kubernetes-watch branch from 1448f21 to 9c63030 Compare December 19, 2017 21:36

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch from bad0e2d to 02e756f Compare December 19, 2017 21:37

igorpeshansky changed the base branch from igorp-kubernetes-refactoring to igorp-kubernetes-memoize-owners December 19, 2017 21:40

igorpeshansky changed the base branch from igorp-kubernetes-memoize-owners to igorp-kubernetes-refactoring December 19, 2017 21:42

igorpeshansky changed the base branch from igorp-kubernetes-refactoring to igorp-kubernetes-memoize-owners December 19, 2017 21:42

igorpeshansky requested a review from supriyagarg December 19, 2017 23:07

igorpeshansky force-pushed the igorp-kubernetes-memoize-owners branch from bad0e2d to 94f1019 Compare December 19, 2017 23:18

qingling128 reviewed Dec 19, 2017

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-watch branch from 9c63030 to e40f01b Compare December 19, 2017 23:31

igorpeshansky changed the base branch from igorp-kubernetes-memoize-owners to igorp-kubernetes-refactoring December 19, 2017 23:31

igorpeshansky force-pushed the igorp-kubernetes-watch branch from e40f01b to 9c63030 Compare December 19, 2017 23:34

igorpeshansky commented Dec 20, 2017

View reviewed changes

qingling128 suggested changes Dec 20, 2017

View reviewed changes

igorpeshansky commented Dec 20, 2017

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch 2 times, most recently from f7c65d9 to 38d3db0 Compare December 20, 2017 19:56

qingling128 approved these changes Dec 20, 2017

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch 2 times, most recently from 02e756f to d623dcb Compare December 20, 2017 22:55

supriyagarg reviewed Dec 20, 2017

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch 2 times, most recently from d20cef7 to d623dcb Compare December 20, 2017 23:04

igorpeshansky changed the base branch from igorp-kubernetes-refactoring to master December 20, 2017 23:07

igorpeshansky force-pushed the igorp-kubernetes-watch branch from d21a6af to a7ce50a Compare December 20, 2017 23:15

igorpeshansky commented Dec 20, 2017

View reviewed changes

igorpeshansky added 12 commits December 20, 2017 18:25

Initial watch implementation.

84457b7

Add watch thread synchronization.

5b45179

Parse all objects in the stream.

21dc954

Logging cleanup.

1eccacc

Move watch callback into KubernetesReader.

70dc72f

Factor out the metadata update callback.

35ae959

Get both pod and container metadata from watch.

67d5f4d

Add a node watch.

9e96f69

Guard watch uses by a config option; allow overriding current node.

d484ecf

Bump version to 0.0.14.k8s.watch-1.

0e4bc9f

Clarify remaining chunk bytes vs available request bytes.

40fc0cf

Handle blank lines in chunked encoding early.

2297cf8

igorpeshansky force-pushed the igorp-kubernetes-watch branch from a7ce50a to 2297cf8 Compare December 20, 2017 23:26

supriyagarg approved these changes Dec 20, 2017

View reviewed changes

Bump version to 0.0.14-1.

815f417

igorpeshansky merged commit d68c701 into master Dec 21, 2017

igorpeshansky deleted the igorp-kubernetes-watch branch December 21, 2017 00:09

Kubernetes watch implementation. #45

Kubernetes watch implementation. #45

Uh oh!

Conversation

igorpeshansky commented Dec 19, 2017

Uh oh!

igorpeshansky commented Dec 19, 2017

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 Dec 19, 2017 • edited by igorpeshansky 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!

bmoyles0117 Dec 19, 2017 • edited by igorpeshansky 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!

bmoyles0117 Dec 19, 2017 • edited by igorpeshansky 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!

igorpeshansky 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

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

qingling128 Dec 19, 2017 •

edited by igorpeshansky

Loading