A set of refactorings to enable Kubernetes watch. #44

igorpeshansky · 2017-12-19T00:40:28Z

Decouple metadata updates from polling.
Factor out common functionality in Kubernetes API response processing.

igorpeshansky · 2017-12-19T00:42:41Z

@supriyagarg, would be great if you could review this as well.
The last change in the description probably deserves to be a separate PR, as it's not really semantics-preserving.

bmoyles0117

First pass, will comb through again.

bmoyles0117 · 2017-12-19T02:14:14Z

src/kubernetes.cc

+
+json::value KubernetesReader::ComputePodAssociations(const json::Object* pod)
+    const throw(json::Exception) {
+  const std::string platform = "gce";  // TODO: detect other platforms.


Should this be GKE, are are we specifically talking about the compute platform? If the compute platform, I think a clarifying comment would help since the rename.

No, this is the platform of the instance on which the node is running on. We'll probably want to remove this anyway, because it's already present in the node metadata.

bmoyles0117 · 2017-12-19T02:18:41Z

src/kubernetes.cc

+      top_level_metadata->Get<json::String>("uid") != pod_id) {
+    LOG(ERROR) << "Internal error; top-level controller without 'kind' "
+               << *top_level_controller
+               << " not the same as pod " << *pod;


Should we be returning out of this, so that we don't assume that the entity we're looking at is a pod?

I think in this case, assuming that it's a pod is fairly harmless and won't break anything downstream. We do log an error in case someone wanted to investigate. Not worth terminating the poll (i.e., throwing an exception) for.

bmoyles0117 · 2017-12-19T02:23:20Z

src/kubernetes.cc

+      kResourceTypeSeparator);
+  return MetadataUpdater::ResourceMetadata(
+      std::vector<std::string>{k8s_pod_id, k8s_pod_name},
+      k8s_pod,


Fits on previous line.

I'm keeping this as one parameter per line, for readability.

bmoyles0117 · 2017-12-19T02:25:10Z

src/kubernetes.cc

+  bool is_deleted = false;
+
+  const MonitoredResource k8s_container("k8s_container", {
+    {"cluster_name", cluster_name},


Building and using these labels seems heavily shared between containers and pods, can we recycle some code here?

We already share the label values. Unfortunately, once you start dynamically building arrays, this stops looking like a literal, which was the whole purpose.

bmoyles0117 · 2017-12-19T02:27:44Z

src/kubernetes.cc

+
+  const MonitoredResource gke_container("gke_container", {
+    {"cluster_name", cluster_name},
+    {"namespace_id", namespace_name},


Caution, pretty sure the legacy resource type is using the namespace uid here, not the name. Just had to deal with this with the integration tests.

Hmm. The logging agent uses the namespace name (and pod name, for that matter) in the legacy resources, as I recall. If anything, I should be changing the pod id to pod name instead...
Good catch, but I think it's orthogonal to this refactoring. Let's open a bug and follow up.

SGTM, I'll file the bug and we'll address these issues there.

bmoyles0117 · 2017-12-19T02:36:44Z

src/kubernetes.cc

+  );
+}
+
+MetadataUpdater::ResourceMetadata KubernetesReader::GetLegacyResource(


Wasn't there a strange hack around using the gke_container resource to monitor instances? Do we need to be capturing those as well?

I think you mean s/instances/nodes/. Yes, there was a hack for nodes (and pods), but it relied on setting the container name appropriately, so we don't have to worry about this -- this code deals with actual containers.

bmoyles0117 · 2017-12-19T02:40:01Z

src/kubernetes.cc

            status->Get<json::Array>("containerStatuses");
-
        if (container_specs->size() != container_list->size()) {
          LOG(ERROR) << "Container specs and statuses arrays "


Do I understand correctly that you are logging an error when the kubernetes pod spec containers array doesn't match the number of pods that actually exist within the pod? If so, it's important to note that this SHOULD be expected. The pod should only not conflict with its parent replicaset, but it doesn't need to be consistent with its parent deployment as it might be undergoing an active change.

Nope, this checks that the containers and container_specs arrays have the same size within a single pod returned by a single API call. If they don't, something's gone terribly wrong.

Is this even possible to occur? If it's such an extreme scenario, are we essentially validating the Kubernetes API at this point?

Yes, kind of. I don't know if it's possible. This is basically an assertion. I didn't want to crash when this occurs, because there's an obvious recovery path, so I just log an error and continue.

bmoyles0117 · 2017-12-19T02:41:06Z

src/kubernetes.cc

-#endif
-          );
+
+        std::vector<MetadataUpdater::ResourceMetadata> pod_metadata =


Nit: I understand the naming, but pod_metadata still led me to believe this was just metadata for pod resource types.

bmoyles0117 · 2017-12-19T02:43:10Z

src/kubernetes.cc

-  return QueryMaster(path_component.first + "/namespaces/" + ns + "/" +
-                     path_component.second + "/" + name);
+    owner = std::move(
+        QueryMaster(path_component.first + "/namespaces/" + ns + "/" +


Why not just use api_version and kind here?

I'm confused. The path_component is based on api_version and kind, but there's more to it.

Sorry, I didn't realize how much KindPath was doing. SGTM.

Yeah, the joys of the Kubernetes Master API spec...

bmoyles0117 · 2017-12-19T02:45:27Z

src/updater.cc

-      store_->UpdateResource(
-          result.ids, result.resource, std::move(result.metadata));
+      UpdateResourceCallback(result);
+      UpdateMetadataCallback(std::move(result));


Unfamiliarity with C++ here, will there be any implications when using move when we start cleaning up resource metadata after being synced? Should resource callback get a copy? Perhaps it already is, just airing concerns.

std::move hands off the ownership of result to the callback. From here on, result does not own any memory.

My concern is that previously you passed in result.resource, and used std::move only on the metadata, whereas now you're moving the entire object and sharing it. Will that have any negative ramifications for the resource callback?

Nope, the callbacks are invoked sequentially, and the resource callback doesn't modify the result (and, in fact, doesn't touch result.metadata). After the metadata callback is invoked, the result's metadata is gone. As for moving the whole result as opposed to result.metadata, that support is added in dc2e1d3.

igorpeshansky

Addressed feedback. PTAL.

igorpeshansky · 2017-12-19T17:00:58Z

src/kubernetes.cc

+
+json::value KubernetesReader::ComputePodAssociations(const json::Object* pod)
+    const throw(json::Exception) {
+  const std::string platform = "gce";  // TODO: detect other platforms.


No, this is the platform of the instance on which the node is running on. We'll probably want to remove this anyway, because it's already present in the node metadata.

igorpeshansky · 2017-12-19T17:08:58Z

src/kubernetes.cc

+      top_level_metadata->Get<json::String>("uid") != pod_id) {
+    LOG(ERROR) << "Internal error; top-level controller without 'kind' "
+               << *top_level_controller
+               << " not the same as pod " << *pod;


I think in this case, assuming that it's a pod is fairly harmless and won't break anything downstream. We do log an error in case someone wanted to investigate. Not worth terminating the poll (i.e., throwing an exception) for.

igorpeshansky · 2017-12-19T17:10:06Z

src/kubernetes.cc

+      kResourceTypeSeparator);
+  return MetadataUpdater::ResourceMetadata(
+      std::vector<std::string>{k8s_pod_id, k8s_pod_name},
+      k8s_pod,


I'm keeping this as one parameter per line, for readability.

igorpeshansky · 2017-12-19T17:11:11Z

src/kubernetes.cc

+  bool is_deleted = false;
+
+  const MonitoredResource k8s_container("k8s_container", {
+    {"cluster_name", cluster_name},


We already share the label values. Unfortunately, once you start dynamically building arrays, this stops looking like a literal, which was the whole purpose.

igorpeshansky · 2017-12-19T17:12:42Z

src/kubernetes.cc

+  );
+}
+
+MetadataUpdater::ResourceMetadata KubernetesReader::GetLegacyResource(


I think you mean s/instances/nodes/. Yes, there was a hack for nodes (and pods), but it relied on setting the container name appropriately, so we don't have to worry about this -- this code deals with actual containers.

igorpeshansky · 2017-12-19T17:18:43Z

src/kubernetes.cc

+
+  const MonitoredResource gke_container("gke_container", {
+    {"cluster_name", cluster_name},
+    {"namespace_id", namespace_name},


Hmm. The logging agent uses the namespace name (and pod name, for that matter) in the legacy resources, as I recall. If anything, I should be changing the pod id to pod name instead...
Good catch, but I think it's orthogonal to this refactoring. Let's open a bug and follow up.

igorpeshansky · 2017-12-19T17:20:02Z

src/updater.cc

-      store_->UpdateResource(
-          result.ids, result.resource, std::move(result.metadata));
+      UpdateResourceCallback(result);
+      UpdateMetadataCallback(std::move(result));


std::move hands off the ownership of result to the callback. From here on, result does not own any memory.

igorpeshansky · 2017-12-19T17:21:11Z

src/kubernetes.cc

            status->Get<json::Array>("containerStatuses");
-
        if (container_specs->size() != container_list->size()) {
          LOG(ERROR) << "Container specs and statuses arrays "


Nope, this checks that the containers and container_specs arrays have the same size within a single pod returned by a single API call. If they don't, something's gone terribly wrong.

igorpeshansky · 2017-12-19T17:24:27Z

src/kubernetes.cc

-#endif
-          );
+
+        std::vector<MetadataUpdater::ResourceMetadata> pod_metadata =


igorpeshansky · 2017-12-19T17:25:27Z

src/kubernetes.cc

-  return QueryMaster(path_component.first + "/namespaces/" + ns + "/" +
-                     path_component.second + "/" + name);
+    owner = std::move(
+        QueryMaster(path_component.first + "/namespaces/" + ns + "/" +


I'm confused. The path_component is based on api_version and kind, but there's more to it.

igorpeshansky

Responded. PTAL.

igorpeshansky · 2017-12-19T18:55:28Z

src/kubernetes.cc

            status->Get<json::Array>("containerStatuses");
-
        if (container_specs->size() != container_list->size()) {
          LOG(ERROR) << "Container specs and statuses arrays "


Yes, kind of. I don't know if it's possible. This is basically an assertion. I didn't want to crash when this occurs, because there's an obvious recovery path, so I just log an error and continue.

igorpeshansky · 2017-12-19T18:56:05Z

src/kubernetes.cc

-  return QueryMaster(path_component.first + "/namespaces/" + ns + "/" +
-                     path_component.second + "/" + name);
+    owner = std::move(
+        QueryMaster(path_component.first + "/namespaces/" + ns + "/" +


Yeah, the joys of the Kubernetes Master API spec...

igorpeshansky · 2017-12-19T18:59:20Z

src/updater.cc

-      store_->UpdateResource(
-          result.ids, result.resource, std::move(result.metadata));
+      UpdateResourceCallback(result);
+      UpdateMetadataCallback(std::move(result));


Nope, the callbacks are invoked sequentially, and the resource callback doesn't modify the result (and, in fact, doesn't touch result.metadata). After the metadata callback is invoked, the result's metadata is gone. As for moving the whole result as opposed to result.metadata, that support is added in dc2e1d3.

bmoyles0117 · 2017-12-19T19:57:46Z

src/kubernetes.cc

-    const std::string k8s_node_id = boost::algorithm::join(
-        std::vector<std::string>{kK8sNodeResourcePrefix, node_id},
-        kResourceTypeSeparator);
+  // TODO: do we need this?


I can't think of a use-case where this would be helpful for now. It's simply enough to add the resource identifier later on that we might as well remove it for now.

bmoyles0117 · 2017-12-19T20:04:26Z

src/kubernetes.cc

+
+  // TODO: find pod_deleted.
+  //const json::Object* status = pod->Get<json::Object>("status");
+  bool pod_deleted = false;


Nit: Rename to is_deleted or rename is_deleted in the container section to container_deleted for consistency.

bmoyles0117 · 2017-12-19T20:10:11Z

src/kubernetes.cc

+  const json::Object* status = pod->Get<json::Object>("status");
+
+  const json::Array* container_specs = spec->Get<json::Array>("containers");
+  const json::Array* container_list =


Confused by the name, can this be container_statuses instead?

Done. Also renamed c_element.

bmoyles0117 · 2017-12-19T20:22:06Z

src/kubernetes.cc

-          );
+
+        std::vector<MetadataUpdater::ResourceMetadata> full_pod_metadata =
+            GetPodAndContainerMetadata(pod, collected_at);


Can you clarify, are we only running subqueries for pods, for the node that we're running on? I can't seem to find where we would list pods that may not be associated to a node due to scheduling restrictions by Kubernetes due to CPU / Memory / Host Ports / etc... Not having a local resource ID to query for is fine, but not syncing metadata would be bad from an observability point of view.

Correct. Unscheduled pods are out of scope for the current iteration. For now we only require metadata for pods/containers that have either logs or metrics associated with them, and all of those would have been scheduled to a node.

bmoyles0117 · 2017-12-19T20:25:02Z

src/updater.cc

-      store_->UpdateResource(
-          result.ids, result.resource, std::move(result.metadata));
+      UpdateResourceCallback(result);
+      UpdateMetadataCallback(std::move(result));


igorpeshansky

Addressed comments. PTAL.

igorpeshansky · 2017-12-19T21:12:41Z

src/kubernetes.cc

-    const std::string k8s_node_id = boost::algorithm::join(
-        std::vector<std::string>{kK8sNodeResourcePrefix, node_id},
-        kResourceTypeSeparator);
+  // TODO: do we need this?


igorpeshansky · 2017-12-19T21:18:05Z

src/kubernetes.cc

+
+  // TODO: find pod_deleted.
+  //const json::Object* status = pod->Get<json::Object>("status");
+  bool pod_deleted = false;


igorpeshansky · 2017-12-19T21:21:08Z

src/kubernetes.cc

+  const json::Object* status = pod->Get<json::Object>("status");
+
+  const json::Array* container_specs = spec->Get<json::Array>("containers");
+  const json::Array* container_list =


Done. Also renamed c_element.

igorpeshansky · 2017-12-19T21:23:41Z

src/kubernetes.cc

-          );
+
+        std::vector<MetadataUpdater::ResourceMetadata> full_pod_metadata =
+            GetPodAndContainerMetadata(pod, collected_at);


Correct. Unscheduled pods are out of scope for the current iteration. For now we only require metadata for pods/containers that have either logs or metrics associated with them, and all of those would have been scheduled to a node.

igorpeshansky · 2017-12-19T21:40:13Z

Pulled out owner memoization into #46.

qingling128 · 2017-12-19T22:27:57Z

src/api_server.cc

  }
+}
+
+void MetadataAgent::UpdateMetadata(const MonitoredResource& resource,


Just curious, why is this second map not keyed by resource_id as well?

Because that map is sent to to the Resource Metadata API, which neither knows nor cares about the local resource ids.

Ah, got it. Makes sense.

Curious - how is the local resource_id different from the monitored resource ID?

The local resource_id is an easily discoverable identifier for the resource that is unique within the current context (e.g., a Kubernetes node), but may not be globally unique, whereas the monitored resource (which actually is an id, so "monitored resource id" is redundant) is globally unique.

…gMetadataUpdater constructor arguments.

…adata. Use separate locks for each map. Make it explicit that UpdateMetadataCallback takes ownership of the result.

…urceMetadata.

igorpeshansky · 2017-12-19T23:06:23Z

Rebased. PTAL.

qingling128

LGTM (Learning how to use github more efficiently LOL)

bmoyles0117 · 2017-12-20T20:07:14Z

src/kubernetes.cc

+  if (config_.VerboseLogging()) {
+    LOG(INFO) << "Container: " << *c_status;
+  }
+  const json::Object* container = c_status->As<json::Object>();


Would renaming this to container_status cause a bunch if line formatting issues? I got confused below when we sent the status metadata and provided the container, would have been more clear if it were container_status. Also throughout.

We'll have to change this code anyway to fix the list ordering issue -- it would make sense to change this in that PR. WDYT?

bmoyles0117 · 2017-12-20T20:08:07Z

src/kubernetes.cc

+        {"version", json::string(kKubernetesApiVersion)},
+        {"raw", container->Clone()},
+      })},
+      {"labels", json::object({


Container labels will never vary from the pod labels, should we really be sending this up? Is this simply a feature request for simplicity on the UI side?

This is an artifact of treating each container as a separate resource, so that's what the API expects.

I'm confused, the node doesn't need to send labels, these are raw kubernetes labels in the raw metadata, not resource labels.

When I say "node doesn't need to", I'm specifically referring to the part way above where the node sends off the raw metadata blob.

Both nodes and pods get the full Kubernetes object metadata, which includes the labels. Containers, on the other hand, have an artificially constructed metadata blob, which, instead of "raw", includes "spec", "status", and "labels" -- that's just the way it was defined. All of these are, in fact, redundant, because they're present in the pod metadata, but because containers are stored separately, the information is replicated. Note that this the way it's been since the start, and is orthogonal to this refactoring.

Alright, agreed that it's orthogonal. SGTM.

bmoyles0117 · 2017-12-20T20:10:22Z

src/kubernetes.cc

+    {"zone", zone},
+  });
+
+  const std::string gke_container_id = boost::algorithm::join(


Shouldn't this be using the container ID instead of pod_id name? If not, can we rename the variable? I know this will likely be deprecated, but caught my eye for now.

bmoyles0117 · 2017-12-20T20:11:52Z

src/kubernetes.cc

-          );
+
+        std::vector<MetadataUpdater::ResourceMetadata> full_pod_metadata =
+            GetPodAndContainerMetadata(pod, collected_at);


igorpeshansky

Addressed feedback. PTAL.

igorpeshansky · 2017-12-20T20:20:07Z

src/kubernetes.cc

+  if (config_.VerboseLogging()) {
+    LOG(INFO) << "Container: " << *c_status;
+  }
+  const json::Object* container = c_status->As<json::Object>();


We'll have to change this code anyway to fix the list ordering issue -- it would make sense to change this in that PR. WDYT?

igorpeshansky · 2017-12-20T20:20:45Z

src/kubernetes.cc

+        {"version", json::string(kKubernetesApiVersion)},
+        {"raw", container->Clone()},
+      })},
+      {"labels", json::object({


This is an artifact of treating each container as a separate resource, so that's what the API expects.

igorpeshansky · 2017-12-20T20:41:25Z

src/kubernetes.cc

+    {"zone", zone},
+  });
+
+  const std::string gke_container_id = boost::algorithm::join(


bmoyles0117

LGTM 🐿

bmoyles0117 · 2017-12-20T21:31:05Z

src/kubernetes.cc

+        {"version", json::string(kKubernetesApiVersion)},
+        {"raw", container->Clone()},
+      })},
+      {"labels", json::object({


Alright, agreed that it's orthogonal. SGTM.

bmoyles0117 · 2017-12-20T21:31:16Z

src/kubernetes.cc

+  if (config_.VerboseLogging()) {
+    LOG(INFO) << "Container: " << *c_status;
+  }
+  const json::Object* container = c_status->As<json::Object>();


supriyagarg · 2017-12-20T21:54:03Z

src/api_server.cc

  }
+}
+
+void MetadataAgent::UpdateMetadata(const MonitoredResource& resource,


Curious - how is the local resource_id different from the monitored resource ID?

supriyagarg · 2017-12-20T21:56:11Z

src/api_server.h

+                      const MonitoredResource& resource);
+
+  // Updates metadata for a given resource.
+  // Adds a metadata mapping from the `resource` to the metadata `entry`.


What does "metadata mapping" mean. Is just "mapping" sufficient?

The metadata agent maintains two mappings:

local resource map, from a locally unique id to the monitored resource (there may be multiple locally unique ids, all referring to the same monitored resource), which is exposed via a local endpoint to other agents so they don't have to discover all parts of the monitored resource themselves, and

metadata map, from the monitored resource to the metadata blob, which is sent to the Resource Metadata API.

supriyagarg · 2017-12-20T21:59:13Z

src/kubernetes.h

+  MetadataUpdater::ResourceMetadata GetContainerMetadata(
+      const json::Object* pod, int container_index, json::value associations,
+      Timestamp collected_at) const throw(json::Exception);
+  // Given a pod object and container index, return the legacy resource.


legacy resource -> legacy container metadata?

There will be no actual metadata in the returned object (see the next line), so this is intentional.

igorpeshansky

@supriyagarg, responded to your comments. PTAL.

igorpeshansky · 2017-12-20T22:42:26Z

src/api_server.cc

  }
+}
+
+void MetadataAgent::UpdateMetadata(const MonitoredResource& resource,


The local resource_id is an easily discoverable identifier for the resource that is unique within the current context (e.g., a Kubernetes node), but may not be globally unique, whereas the monitored resource (which actually is an id, so "monitored resource id" is redundant) is globally unique.

igorpeshansky · 2017-12-20T22:45:53Z

src/api_server.h

+                      const MonitoredResource& resource);
+
+  // Updates metadata for a given resource.
+  // Adds a metadata mapping from the `resource` to the metadata `entry`.


The metadata agent maintains two mappings:

local resource map, from a locally unique id to the monitored resource (there may be multiple locally unique ids, all referring to the same monitored resource), which is exposed via a local endpoint to other agents so they don't have to discover all parts of the monitored resource themselves, and

metadata map, from the monitored resource to the metadata blob, which is sent to the Resource Metadata API.

igorpeshansky · 2017-12-20T22:46:57Z

src/kubernetes.h

+  MetadataUpdater::ResourceMetadata GetContainerMetadata(
+      const json::Object* pod, int container_index, json::value associations,
+      Timestamp collected_at) const throw(json::Exception);
+  // Given a pod object and container index, return the legacy resource.


There will be no actual metadata in the returned object (see the next line), so this is intentional.

supriyagarg

Thanks for the clarifications Igor.

LGTM.

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

bmoyles0117 reviewed Dec 19, 2017

View reviewed changes

igorpeshansky commented Dec 19, 2017

View reviewed changes

bmoyles0117 reviewed Dec 19, 2017

View reviewed changes

igorpeshansky commented Dec 19, 2017

View reviewed changes

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

qingling128 reviewed Dec 19, 2017

View reviewed changes

igorpeshansky added 7 commits December 19, 2017 17:47

New DockerUpdater and KubernetesUpdater classes. Swap order of Pollin…

4bec5c7

…gMetadataUpdater constructor arguments.

Factor out the MetadataUpdater abstract class.

157d9f4

Split MetadataAgent::UpdateResource into UpdateResource and UpdateMet…

00a9141

…adata. Use separate locks for each map. Make it explicit that UpdateMetadataCallback takes ownership of the result.

Factor out metadata creation.

6118758

Factor out GetPodAndContainerMetadata; add a move constructor to Reso…

6e91a2d

…urceMetadata.

Rename local variable.

a3258f5

Address feedback.

38d3db0

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch from 02e756f to 38d3db0 Compare December 19, 2017 23:03

igorpeshansky changed the base branch from igorp-miscellaneous-cleanup to master December 19, 2017 23:04

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

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

qingling128 approved these changes Dec 20, 2017

View reviewed changes

bmoyles0117 suggested changes Dec 20, 2017

View reviewed changes

Rename variable.

d623dcb

igorpeshansky commented Dec 20, 2017

View reviewed changes

bmoyles0117 approved these changes Dec 20, 2017

View reviewed changes

supriyagarg reviewed Dec 20, 2017

View reviewed changes

igorpeshansky force-pushed the igorp-kubernetes-refactoring branch from d623dcb to 02e756f Compare December 20, 2017 22:39

igorpeshansky commented Dec 20, 2017

View reviewed changes

supriyagarg approved these changes Dec 20, 2017

View reviewed changes

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

igorpeshansky merged commit 277e6ba into master Dec 20, 2017

igorpeshansky deleted the igorp-kubernetes-refactoring branch January 23, 2018 20:15

A set of refactorings to enable Kubernetes watch. #44

A set of refactorings to enable Kubernetes watch. #44

Uh oh!

Conversation

igorpeshansky commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorpeshansky commented Dec 19, 2017

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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!

igorpeshansky commented Dec 19, 2017 •

edited

Loading

igorpeshansky Dec 19, 2017 •

edited

Loading

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

bmoyles0117 Dec 19, 2017 •

edited by igorpeshansky

Loading

igorpeshansky Dec 19, 2017 •

edited

Loading