Add support for collecting services. #112

supriyagarg · 2018-03-30T22:57:18Z

Will add unit tests in the same PR. Would like your feedback on the code first.

igorpeshansky

The basic implementation structure looks good.

igorpeshansky · 2018-03-31T04:11:26Z

src/kubernetes.cc

  return std::move(result);
 }

+


No need for an extra blank line here.

igorpeshansky · 2018-03-31T04:12:34Z

src/kubernetes.cc

+
+std::vector<json::value> KubernetesReader::GetServiceList(
+    const std::string cluster_name, const std::string location
+) const throw(json::Exception) {


It's better to format this as:

std::vector<json::value> KubernetesReader::GetServiceList( const std::string& cluster_name, const std::string& location) const throw(json::Exception) {

igorpeshansky · 2018-03-31T04:12:50Z

src/kubernetes.cc


+
+std::vector<json::value> KubernetesReader::GetServiceList(
+    const std::string cluster_name, const std::string location


const std::string& in both cases.

igorpeshansky · 2018-03-31T04:20:16Z

src/kubernetes.cc

+) const throw(json::Exception) {
+  std::lock_guard<std::recursive_mutex> lock(service_mutex_);
+  std::vector<json::value> service_list;
+  for (auto const& service_it : service_to_metadata_) {


const auto& is more conventional...

igorpeshansky · 2018-03-31T04:21:41Z

src/kubernetes.h

  // A memoized map from an encoded owner reference to the owner object.
  mutable std::map<std::string, json::value> owners_;
+  // Mutex for the service related caches.
+  mutable std::recursive_mutex service_mutex_;


Do you expect to ever call a function while holding this lock that would need to re-acquire the lock? If not, you can just make this an std::mutex instead.

changed - it is not being re-acquired right now.

igorpeshansky · 2018-03-31T05:12:53Z

src/kubernetes.cc

+
+  // TODO: using a temporary did not work here.
+  std::vector<MetadataUpdater::ResourceMetadata> result_vector;
+  result_vector.emplace_back(GetClusterMetadata(collected_at, is_deleted));


Careful — this is_deleted refers to the service, right? You don't want to notify the API that the whole cluster has been deleted just because one service was. I would suggest just not having an is_deleted parameter in GetClusterMetadata.

igorpeshansky · 2018-03-31T05:16:57Z

src/kubernetes.cc

+    WatchMaster(
+        "Service",
+        std::string(kKubernetesEndpointPath) + "/watch/services/",
+        [=](const json::Object* service, Timestamp collected_at, bool is_deleted) {


Fit in 80 columns?

fixed - it was too long

igorpeshansky · 2018-03-31T05:19:50Z

src/kubernetes.cc

+
+  // TODO: using a temporary did not work here.
+  std::vector<MetadataUpdater::ResourceMetadata> result_vector;
+  result_vector.emplace_back(GetClusterMetadata(collected_at, is_deleted));


Same here — this is_deleted refers to the endpoints object.

igorpeshansky · 2018-03-31T05:20:36Z

src/kubernetes.cc

+    MetadataUpdater::UpdateCallback callback,
+    const json::Object* endpoints, Timestamp collected_at, bool is_deleted)
+    throw(json::Exception) {
+  UpdateServiceToPodsCache(endpoints, is_deleted);


What does it mean for an endpoint to be deleted? Can one be deleted without the corresponding service being deleted, or are they always correlated?

the two are correlated - endpoints is deleted when the service is deleted.

igorpeshansky · 2018-03-31T05:23:52Z

src/kubernetes.cc

    pod_watch_thread_ = std::thread([=]() {
      reader_.WatchPods(watched_node, cb);
    });
+    if (config().KubernetesClusterLevelMetadata()) {


I wonder if it makes sense to define an extra option just for service metadata? Or will we always want to retrieve it along with unscheduled pods?

Added a separate option. I wonder how the the two flags interact - i.e. if KubernetesClusterLevelMetadata is false, does it make sense for KubernetesServiceMetadata to be true?

Well, KubernetesClusterLevelMetadata means it's running the separate instance at the cluster level, rather than the per-node one. There are many things we can watch at cluster level. KubernetesServiceMetadata controls specifically whether we watch services/endpoints at cluster level. So KubernetesServiceMetadata is ignored when KubernetesClusterLevelMetadata is false, but it can certainly be set to true. See my other comment about guarding the watch threads.

igorpeshansky · 2018-03-31T16:06:14Z

src/kubernetes.cc

+    auto endpoints_it = service_to_pods_.find(service_key);
+    const std::vector<std::string>& pod_names =
+        (endpoints_it != service_to_pods_.end()) ? endpoints_it->second
+                                             : kNoPods;


Something's off with the indentation here...

aligned the "?" and ":"

igorpeshansky · 2018-03-31T16:07:17Z

src/kubernetes.h

+  void UpdateServiceToPodsCache(
+      const json::Object* endpoints, bool is_deleted) throw(json::Exception);
+
+  // Const data.


This isn't helpful. How about // An empty vector value for endpoints that have no pods.?

igorpeshansky · 2018-03-31T16:08:58Z

src/kubernetes.cc

+      k8s_cluster,
+#ifdef ENABLE_KUBERNETES_METADATA
+      MetadataStore::Metadata(config_.MetadataIngestionRawContentVersion(),
+                              /*is_deleted=*/ false, created_at, collected_at,


No space after comment (so it reads /*is_deleted=*/false).

igorpeshansky · 2018-03-31T16:10:38Z

src/kubernetes.cc

+  const json::Object* metadata = service->Get<json::Object>("metadata");
+  const std::string namespace_name = metadata->Get<json::String>("namespace");
+  const std::string service_name = metadata->Get<json::String>("name");
+  const std::string encoded_ref = boost::algorithm::join(


D'oh. Even better! Yes, please!

igorpeshansky · 2018-03-31T16:12:19Z

src/kubernetes.cc

+  } else {
+    auto it_inserted =
+        service_to_pods_.emplace(encoded_ref, std::vector<std::string>());
+    service_to_pods_.at(encoded_ref) = pod_names;


Let's avoid the extra lookup, and use it_inserted.first->second instead.

igorpeshansky · 2018-03-31T16:29:12Z

src/kubernetes.h

+  // Return the cluster metadata based on the cached values for
+  // service_to_metadta_ and service_to_pods_.
+  MetadataUpdater::ResourceMetadata GetClusterMetadata(
+      Timestamp collected_at) const throw(json::Exception);


This can be:

MetadataUpdater::ResourceMetadata GetClusterMetadata(Timestamp collected_at) const throw(json::Exception);

igorpeshansky · 2018-03-31T16:30:15Z

src/configuration.cc

      kubernetes_cluster_level_metadata_(
          kKubernetesDefaultClusterLevelMetadata),
+      kubernetes_service_metadata_(
+          kKubernetesDefaultServiceMetadata),


Fits on one line.

igorpeshansky · 2018-03-31T16:31:44Z

src/kubernetes.cc

    pod_watch_thread_ = std::thread([=]() {
      reader_.WatchPods(watched_node, cb);
    });
+    if (config().KubernetesServiceMetadata()) {


This actually needs both config().KubernetesClusterLevelMetadata() && config().KubernetesServiceMetadata().

igorpeshansky · 2018-03-31T16:35:46Z

src/kubernetes.cc

+    if (service_it == service_to_pods_.end()) {
+      service_to_pods_.emplace(encoded_ref, pod_names);
+    } else {
+      service_it->second = pod_names;


Ah, you're right. I forgot that we have to re-construct the k8s_cluster resource each time. Oh, well.
I am wondering whether the cost of copying large vectors around would be comparable with the cost of constructing the vector in-place, but we can defer it to later (or, optionally, add a TODO to investigate).

igorpeshansky · 2018-03-31T16:39:00Z

src/kubernetes.cc

    pod_watch_thread_ = std::thread([=]() {
      reader_.WatchPods(watched_node, cb);
    });
+    if (config().KubernetesClusterLevelMetadata()) {


Well, KubernetesClusterLevelMetadata means it's running the separate instance at the cluster level, rather than the per-node one. There are many things we can watch at cluster level. KubernetesServiceMetadata controls specifically whether we watch services/endpoints at cluster level. So KubernetesServiceMetadata is ignored when KubernetesClusterLevelMetadata is false, but it can certainly be set to true. See my other comment about guarding the watch threads.

igorpeshansky · 2018-04-01T00:31:17Z

src/kubernetes.h

+  mutable std::mutex service_mutex_;
+  // Map from service key to service metadata. This map is built based on the
+  // response from WatchServices.
+  mutable std::map<std::pair<std::string, std::string>,


[Optional] You can define a private helper type:

using ServiceKey = std::pair<std::string, std::string>; // Map from service key to service metadata. This map is built based on the // response from WatchServices. mutable std::map<ServiceKey, json::value> service_to_metadata_; // Map from service key to names of pods in the service. This map is built // based on the response from WatchEndpoints. mutable std::map<ServiceKey, std::vector<std::string>> service_to_pods_;

igorpeshansky · 2018-04-01T00:32:06Z

src/kubernetes.h

      version_to_kind_to_name_;
  // A memoized map from an encoded owner reference to the owner object.
  mutable std::map<std::string, json::value> owners_;
+  // Mutex for the service related caches.


Let's add a blank line before this group of variables.

igorpeshansky · 2018-04-01T00:33:47Z

src/kubernetes.cc

+  const json::Object* metadata = service->Get<json::Object>("metadata");
+  const std::string namespace_name = metadata->Get<json::String>("namespace");
+  const std::string service_name = metadata->Get<json::String>("name");
+  const std::pair<std::string, std::string> service_key (


No space before the ('.

igorpeshansky · 2018-04-01T00:33:53Z

src/kubernetes.cc

+  const std::string namespace_name = metadata->Get<json::String>("namespace");
+  // Endpoints name is same as the matching service name.
+  const std::string service_name = metadata->Get<json::String>("name");
+  const std::pair<std::string, std::string> service_key (


No space before the ('.

igorpeshansky · 2018-04-01T00:37:25Z

src/configuration.cc

 constexpr const char kKubernetesDefaultNodeName[] = "";
 constexpr const bool kKubernetesDefaultUseWatch = true;
 constexpr const bool kKubernetesDefaultClusterLevelMetadata = false;
+constexpr const bool kKubernetesDefaultServiceMetadata = false;


[Optional] Now that you have an additional guard, this can default to true...

since the cluster level flag is already false, prefer to have it be the same.

This would mean that users would have to enable cluster-level watches and the service watches. I would have thought they'd want services by default if they're running at cluster level...
Note: I just turned KubernetesUseWatch off by default, so that's an additional guard, too. So, back to the original question: if a customer runs the agent and explicitly turns on both KubernetesUseWatch and KubernetesClusterLevelMetadata, should they expect to see service metadata? I would guess yes.

igorpeshansky · 2018-04-01T00:56:14Z

src/configuration.cc

 constexpr const char kKubernetesDefaultNodeName[] = "";
 constexpr const bool kKubernetesDefaultUseWatch = true;
 constexpr const bool kKubernetesDefaultClusterLevelMetadata = false;
+constexpr const bool kKubernetesDefaultServiceMetadata = false;


This would mean that users would have to enable cluster-level watches and the service watches. I would have thought they'd want services by default if they're running at cluster level...
Note: I just turned KubernetesUseWatch off by default, so that's an additional guard, too. So, back to the original question: if a customer runs the agent and explicitly turns on both KubernetesUseWatch and KubernetesClusterLevelMetadata, should they expect to see service metadata? I would guess yes.

igorpeshansky · 2018-04-01T00:58:13Z

src/kubernetes.h

+  // Mutex for the service related caches.
+  mutable std::mutex service_mutex_;
+
+  using ServiceKey = std::pair<std::string, std::string>;


Let's just put this type before the mutex and remove the blank line between them (i.e., have one blank line that delimits this type and the data from the previous data fields).

igorpeshansky

LGTM

igorpeshansky

Reviewing tests.

igorpeshansky · 2018-04-01T03:26:00Z

test/kubernetes_unittest.cc

+
+  void UpdateServiceToMetadataCache(
+      KubernetesReader& reader, const json::Object* service,
+      bool is_deleted)


Fits on the previous line.

igorpeshansky · 2018-04-01T03:26:07Z

test/kubernetes_unittest.cc

+
+  void UpdateServiceToPodsCache(
+      KubernetesReader& reader, const json::Object* endpoints,
+      bool is_deleted)


Fits on the previous line.

igorpeshansky · 2018-04-01T03:32:15Z

test/kubernetes_unittest.cc

+  }
+
+  void UpdateServiceToMetadataCache(
+      KubernetesReader& reader, const json::Object* service,


Let's prefer pointers to non-const references (style guide). Also in UpdateServiceToPodsCache.

igorpeshansky · 2018-04-01T03:33:01Z

test/kubernetes_unittest.cc

 }
+
+TEST_F(KubernetesTest, GetClusterMetadataEmpty) {
+  Configuration config(std::stringstream(


std::istringstream everywhere.

igorpeshansky · 2018-04-01T03:33:42Z

test/kubernetes_unittest.cc

+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  const auto m = GetClusterMetadata(reader, Timestamp());
+  EXPECT_EQ(0, m.ids().size());


EXPECT_TRUE(m.ids().empty()). Also below.

igorpeshansky · 2018-04-01T03:34:21Z

test/kubernetes_unittest.cc

+    {"location", "TestClusterLocation"},
+  }), m.resource());
+  EXPECT_EQ("TestVersion", m.metadata().version);
+  EXPECT_EQ(false, m.metadata().is_deleted);


EXPECT_FALSE(m.metadata().is_deleted). Also below.

igorpeshansky · 2018-04-01T03:35:05Z

test/kubernetes_unittest.cc

+    {"metadata", json::object({
+      {"name", json::string("testname")},
+      {"namespace", json::string("testnamespace")},
+    })}


Let's keep the trailing comma.

igorpeshansky · 2018-04-01T03:40:09Z

test/kubernetes_unittest.cc

+    {"metadata", json::object({
+      {"name", json::string("testname")},
+      {"namespace", json::string("testnamespace")},
+    })}


Trailing comma.

igorpeshansky · 2018-04-01T03:41:15Z

test/kubernetes_unittest.cc

+        json::object({
+          {"api", json::object({
+            {"pods", json::array({
+                pod_mr.ToJSON(),


2-space indent.

igorpeshansky · 2018-04-01T03:42:32Z

test/kubernetes_unittest.cc

+  });
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  UpdateServiceToMetadataCache(
+      reader, service->As<json::Object>(), /*is_deleted=*/false);


Do we need to also test deleted services in a separate test?

added a separate test.

igorpeshansky · 2018-04-01T04:24:05Z

test/kubernetes_unittest.cc

+  UpdateServiceToMetadataCache(
+      &reader, service->As<json::Object>(), /*is_deleted=*/false);
+  const auto m = GetClusterMetadata(reader, Timestamp());
+  EXPECT_EQ(0, m.ids().size());


EXPECT_TRUE(m.ids().empty()).

igorpeshansky

LGTM

bmoyles0117 · 2018-03-31T17:38:00Z

src/kubernetes.cc

+  std::vector<json::value> service_list;
+  for (const auto& service_it : service_to_metadata_) {
+    const std::string& service_key = service_it.first;
+    const std::string namespace_name =


Adding a comment that explains what we're extracting would be super helpful.

Done - both here, and a line in the header file.

bmoyles0117 · 2018-04-01T16:13:29Z

test/configuration_unittest.cc

  EXPECT_EQ("", config.KubernetesClusterLocation());
  EXPECT_EQ("", config.KubernetesNodeName());
  EXPECT_EQ(true, config.KubernetesUseWatch());
+  EXPECT_EQ(false, config.KubernetesClusterLevelMetadata());


Should we rename this to KubernetesClusterMetadata? I'm not sure what Level is implying.

This flag is not new in this PR - it is basically set to True if users want cluster level watch for unscheduled pods + service metadata.

The one added here is below: KubernetesServiceMetadata(). I was just updating the unittest to take care of a missing default flag.

I agree that we need documentation around their usage.

"Cluster-level" is the opposite of "node-level". It includes, e.g., unscheduled pods, which are not part of the cluster resource metadata.

bmoyles0117 · 2018-04-01T16:56:39Z

src/kubernetes.cc

+}
+
+void KubernetesReader::UpdateServiceToPodsCache(
+    const json::Object* endpoints, bool is_deleted) throw(json::Exception) {


Endpoints represents a single endpoint, could we rename this to be singular throughout?

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#endpoints-v1-core

The resource is called Endpoints, not Endpoint, since it lists all endpoints used by a single service.

Added comments to this method in the header file.

bmoyles0117 · 2018-04-01T16:58:01Z

src/kubernetes.cc

+  }
+}
+
+void KubernetesReader::UpdateServiceToPodsCache(


I'm a little concerned calling this UpdateServiceToPodsCache, a service itself doesn't have pods, the endpoint does. I may have missed a lot of the conversation, but I'm of the opinion that we should address endpoints as endpoints internally, even if we end up mapping it to a service at the end.

It really is the service that has the pods - Endpoints is just an API that provides the mapping. According to the docs:
Kubernetes offers a simple Endpoints API that is updated whenever the set of Pods in a Service changes.

bmoyles0117 · 2018-04-01T17:05:55Z

src/kubernetes.cc

+    const json::value& service_metadata = service_it.second;
+    auto endpoints_it = service_to_pods_.find(service_key);
+    const std::vector<std::string>& pod_names =
+        (endpoints_it != service_to_pods_.end()) ? endpoints_it->second


This is an extension of what caused my confusion, I feel like it would be clearer to retain "endpoints_to_pods_", with a shared service key, so that this line makes sense when we read it here.

see the other comment - the pods really belong to the service, so I prefer to keep the name.

Hope the renaming of the local iterators helps - at this point the source matters less, we should just be referring to the caches with what they contain.

igorpeshansky

Looks even better!

igorpeshansky · 2018-04-01T19:13:26Z

test/configuration_unittest.cc

  EXPECT_EQ("", config.KubernetesClusterLocation());
  EXPECT_EQ("", config.KubernetesNodeName());
  EXPECT_EQ(true, config.KubernetesUseWatch());
+  EXPECT_EQ(false, config.KubernetesClusterLevelMetadata());


"Cluster-level" is the opposite of "node-level". It includes, e.g., unscheduled pods, which are not part of the cluster resource metadata.

igorpeshansky · 2018-04-01T20:00:47Z

src/kubernetes.h

  // A memoized map from an encoded owner reference to the owner object.
  mutable std::map<std::string, json::value> owners_;

+  // Unique identifier of a service in a cluster, based on the namespace name


How about: // ServiceKey is a pair of the namespace name and the service name that uniquely identifies a service in a cluster.?

igorpeshansky · 2018-04-01T20:03:12Z

src/kubernetes.cc

+  std::lock_guard<std::mutex> lock(service_mutex_);
+  std::vector<json::value> service_list;
+  for (const auto& metadata_it : service_to_metadata_) {
+    // service_key is a std::pair containing (namespace_name, service_name).


This seems really redundant... You could have said something like: // The namespace name is the first component of the service key., but even that basically replicates the code in line 464.
Maybe just // A service key consists of a namespace name and a service name.?

igorpeshansky

LGTM

bmoyles0117

LGTM

…_to_pods_

igorpeshansky · 2018-04-03T22:27:30Z

Looks like you need to rebase again.

supriyagarg · 2018-04-03T22:31:31Z

Rebased and tested locally.

supriyagarg requested review from bmoyles0117 and igorpeshansky March 30, 2018 22:57

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

supriyagarg force-pushed the service_metadata_streaming branch from e0ba7df to 202503c Compare March 31, 2018 16:01

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

igorpeshansky suggested changes Apr 1, 2018

View reviewed changes

igorpeshansky approved these changes Apr 1, 2018

View reviewed changes

igorpeshansky suggested changes Apr 1, 2018

View reviewed changes

supriyagarg force-pushed the service_metadata_streaming branch from ce9b650 to 4865a82 Compare April 1, 2018 04:22

igorpeshansky suggested changes Apr 1, 2018

View reviewed changes

igorpeshansky approved these changes Apr 1, 2018

View reviewed changes

bmoyles0117 suggested changes Apr 1, 2018

View reviewed changes

igorpeshansky approved these changes Apr 1, 2018

View reviewed changes

igorpeshansky suggested changes Apr 1, 2018

View reviewed changes

igorpeshansky mentioned this pull request Apr 1, 2018

Bump version to 0.0.19-1. #116

Merged

igorpeshansky approved these changes Apr 1, 2018

View reviewed changes

supriyagarg changed the base branch from igorp-upstream-watch-chunk to master April 2, 2018 21:27

supriyagarg force-pushed the service_metadata_streaming branch from 7240c18 to 1dab2f2 Compare April 2, 2018 22:05

bmoyles0117 approved these changes Apr 3, 2018

View reviewed changes

supriyagarg added 10 commits April 3, 2018 17:40

Add support for collecting services.

44fc687

Address comments

5fa6771

Add a config option for turning on service metadata

9a20bed

Update unit test for configuration.cc

baed174

Address comments 2

96f9293

address comments 3

8ed1b51

Set KubernetesServiceMetadata to true, minor header fixes.

7c627be

Edit update to service_to_metadata_ to be similar to code for service…

59f860c

…_to_pods_

Add unit tests for GetClusterMetadata

b3ddb0f

Add test for deleted service + address comments

2d25fd9

supriyagarg added 5 commits April 3, 2018 17:47

Fix test to use empty()

d1351e3

Add comments, and rename variables to clarify endpoints vs service

38524ce

Add comments for the ServiceKey

67c1567

Update comments

85a4b27

Fix mismatched braces introduced during rebase

2a7223c

supriyagarg force-pushed the service_metadata_streaming branch from 1dab2f2 to 2a7223c Compare April 3, 2018 22:30

igorpeshansky merged commit fe316cb into Stackdriver:master Apr 3, 2018

supriyagarg deleted the service_metadata_streaming branch April 3, 2018 23:23

igorpeshansky mentioned this pull request Apr 4, 2018

Fix merge conflict in kubernetes_unittest. #126

Merged



		std::vector<json::value> KubernetesReader::GetServiceList(
		const std::string cluster_name, const std::string location

Add support for collecting services. #112

Add support for collecting services. #112

Uh oh!

Conversation

supriyagarg commented Mar 30, 2018

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

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!

supriyagarg Mar 31, 2018 • 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!

igorpeshansky Mar 31, 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

supriyagarg Mar 31, 2018 •

edited by igorpeshansky

Loading

igorpeshansky Mar 31, 2018 •

edited

Loading