Replicaset awareness #107

KnVerey · 2017-06-05T18:35:50Z

@Shopify/cloudplatform

This PR makes KubernetesResource::Deployment aware of replica sets. This should fix a number of outstanding issues:

Pod warnings get shown for pods that are being shut down, in the case where the last deploy was bad and the current one is actually succeeding (very confusing output)
The way we try to identify a representative pod for the purpose of outputting related events and logs on failed deploy is sketchy (Rework deploy output #98 (comment)) and could theoretically still select a pod from the wrong RS.
Rolling updates currently cannot fail, and will never time out on the basis of their pod states (notes in code)

Fixes #36
Fixes #42 (with small amount of extra code + test)

Problem

We currently make the link between a deployment and its pods by looking for a "name" label. This is super fragile, since we're not using the full selector set, plus the presence of that label is a Shopify convention, not a k8s requirement. Furthermore, even using the full selector set does not guarantee you're looking at the right pods if you've gotten yourself into a really bad state (selector overlap). Ideally we'd connect Deployments to the pods we care about in a robust way; this is much easier to do as of Kubernetes 1.6 (see "Additional context" below).

Solution

This PR uses the full set of label selectors to grab all possibly relevant RS. It then uses the new ownerReferences information to select the RS it owns, and the deployment.kubernetes.io/revision to select the current RS. That RS in turn uses label selectors and ownerReferences to list its pods.

Additional context

kubectl itself identifies the new replicaset by calculating and comparing pod template hashes: https://github.com/kubernetes/kubernetes/blob/a9f5065833d782ea93a6f462674fc7d3fb3affe7/pkg/controller/deployment/util/deployment_util.go#L611-L622. I don' know whether they plan to switch to the new attributes I'm using here, but either way, that approach is not feasible for us.
ownerReferences were added to Deployment in 1.6. The PR that introduced them mentions they will be used for RS garbage collection. However, the issue it resolved mentions use cases like ours here.
The deployment.kubernetes.io/revision annotation is much older (circa 1.2). According to the PR introducing it, the rollout history and rollback features leverage it.

KnVerey · 2017-06-05T20:21:45Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+    end
+
+    def deploy_failed?
+      @pods.present? && @pods.all?(&:deploy_failed?)


This (and deploy_timed_out?) still will basically never fail on account of the pods, since the Pod timeout is higher and the failure condition is super narrow (@phase == "Failed"). However, my next task is to make pod failures smarter / more aggressive, and at that point this should make a huge difference in how often our users hit the Deployment timeout.

KnVerey · 2017-06-05T20:24:11Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

-      own_events.merge(@representative_pod.fetch_events)
+    def fetch_logs
+      return {} unless @latest_rs.present?
+      @latest_rs.fetch_logs


kubectl get logs deployment/web -c nginx (or whatever) is a valid command. However, looking at the source, I think it might favour pods from the old RS under some circumstances, and certainly won't exclude them. https://github.com/kubernetes/kubernetes/blob/dbd1503b6546b0b6048787bad0e63515f3281506/pkg/controller/controller_utils.go#L669-L698

AFAIK there's no kubectl get logs, you probably meant kubectl logs. Though I agree that kubectl logs deployment/web -c web can be ambiguous.

How did it work before for a deployment with 100 replicas (== pods)? Would representative_pod be one of a hundred?

Do we fetch logs for the developers to tell them what went wrong?

Yes, I meant kubectl logs. And yes, we're only fetching logs as debug information for these resource types when they fail (unlike unmanaged pods, for which we always dump logs). representative_pod was a hack to be able to introduce the debug output without requiring this PR to be ready in addition to the extensive changes that did go in at the time. It attempted to select a pod from the new replicaSet by looking at pod creationTimestamps.

KnVerey · 2017-06-05T20:26:38Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+      own_events.merge(@pods.first.fetch_events)
+    end
+
+    def fetch_logs


See the source for how kubectl selects which pod to dump logs from. The gist is that it is trying to select a pod that is more likely to actually have logs.

❤️ for checking it and for the reference to k8s source that was not easy to find

KnVerey · 2017-06-05T20:29:41Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

-      @representative_pod ? @representative_pod.fetch_logs : {}
+    def fetch_events
+      return {} unless @latest_rs.present?
+      @latest_rs.fetch_events


We could include deployment events in here too, but I don't think they'd generally be useful (typically, they're about "Scaled replicaSet foo"), and there can be a ton of them since fetch_events doesn't filter by time right now (though I'm considering adding that).

My other PR adds filtering by time, so we can actually keep these events after all.

mkobetic · 2017-06-12T20:50:38Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+          "--since-time=#{@deploy_started.to_datetime.rfc3339}",
+          "--tail=#{LOG_LINE_COUNT}"
+        )
+        container_logs["#{id}/#{container_name}"] = out


Given that we're displaying logs for all the containers in the pod, might make sense to display shorter ones first to avoid having a chatty container drown out a less chatty and possibly more relevant one. Visually separating individual container logs would probably be helpful as well.

How about this?

each gets its own section, with a heading instead of tags on every line

sorted by line count

identifier format container 'foo' instead of the full resource path (Deployment/web/foo)

❤️ 👍

camilo

Bunch of nitpicks I like the idea tho

camilo · 2017-06-12T21:26:33Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

-          .slice("updatedReplicas", "replicas", "availableReplicas", "unavailableReplicas")
-        @status = @rollout_data.map { |replica_state, num| "#{num} #{replica_state}" }.join(", ")
+        deployment_data = JSON.parse(raw_json)
+        latest_rs_data = get_latest_rs(deployment_data)


This violates SLA a bit IMO, why not have something like latest_rs(deployment_data) return an already formed ReplicaSet?

Or find_latest_rs since this method actually gets multiple RS and finds the right one.

camilo · 2017-06-13T00:11:16Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

      @found = st.success?
-      @rollout_data = {}
+      @rollout_data = { "replicas" => 0 }


Do we expect many calls to sycn on the same instance? or should this be set in the constructor?

Yes, sync will be called many, many times on the same instance--about once every three seconds from the beginning of the deploy until the time the resource succeeds/fails. The exception to this is resources that were created to model subresources of the thing that was actually in the template set, e.g. managed replica sets or pods. We'll get new instances of those on every sync.

camilo · 2017-06-13T00:34:55Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+      @parent = parent
+      @logger = logger
+      @pods = []
+      @rollout_data = { "replicas" => 0 }


Oh interesting, this one does do the intialization here, there must be a reason I'm not seeing.

The reason has to do with the possible origins of these resource types:

Deployments always come from a file in your deploy directory and get their observed state via sync.

ReplicaSets usually show up via deployments, but it is possible to deploy them separately, so I've supported that here (and added a test).

Pods are very commonly created both ways: unmanaged from a file in your deploy directory, or via a deployment/replicaSet.

For deployments, we have it easy: we see the file, create the instance, and then call sync on it repeatedly to reset and update the observed state. For the other two, there is the additional possibility that they are being created when their parent syncs, and that the parent already has the data on their observed state. In that case, we create the instance and then call interpret_json_data with that data; sync is never called. Effectively, interpret_json_data is the common core between the two flows. I'll investigate making it private / clarifying these two flows.

camilo · 2017-06-13T00:35:52Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+      end
+    end
+
+    def interpret_json_data(rs_data)


Does this method need to be public?

I get it now, because the data is set into the objects after intialization, as I mentioned in the other comment, sounds like it could be an optional param to the constructor.

camilo · 2017-06-13T00:36:30Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+    end
+
+    def interpret_json_data(rs_data)
+      pods_data = get_pods(rs_data)


I might be argued out of this one, but same as in the RS class, this should probably just return the objects.

camilo · 2017-06-13T00:39:36Z

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -192,5 +193,10 @@ def pretty_status
    def kubectl
      @kubectl ||= Kubectl.new(namespace: @namespace, context: @context, logger: @logger, log_failure_by_default: false)
    end
+
+    def template


Why is this needed, and why is it okay to return nil if the file is not present?

I put it here because multiple subclasses in this PR use it:

ReplicaSet uses it to grab the container names in the case where it was not derived from a deployment (managed ReplicaSets will not have a template file)

Service uses it to find the related deployment so it can be smarter about how many endpoints to expect (notably so that it can expect zero when appropriate)

Your comment make me think that perhaps I should move the fallback right into this method: if we have a file, load it from file; if we don't, fetch it from the API.

I like the change; should it be memoized? or would that be roflscale ?

Memoizing should be safe... From file, the result obviously won't change. Although resources do change cluster-side, the ones that don't have a file are child resources for which that shouldn't be true. For example, if the initial status check (before the deploy makes any changes) on a deployment resource memoized a template from the cluster, that'd be bad, since that template would change when the deploy proceeds. But the child resource objects are replaced by their parents every time the parent syncs.

camilo · 2017-06-13T00:43:36Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

+          parent: "#{@name.capitalize} deployment",
+          logger: @logger
+        )
+        @latest_rs.interpret_json_data(latest_rs_data)


I even think this should be an optional(?) param to the constructor:

ReplicaSet.new(blah blah... data: json)

That ^ or @latest_rs = ReplicaSet.from_json(...)

I remember trying the ReplicaSet.from_json model at one point in this work and running into difficulties... but I don't remember what they were, so I'll try again. :P

camilo · 2017-06-13T00:45:28Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

      @status = nil
-      @representative_pod = nil
-      @pods = []
+      @latest_rs = nil

      if @found


I know is not a change here, but why is there no defined behaviour for if not found?

The behaviour in that case is a reset, which is accomplished by the code above. Basically, we always reset, and then if we find new data, we repopulate using it.

kirs · 2017-06-13T03:51:58Z

test/fixtures/hello-cloud/bare_replica_set.yml

@@ -0,0 +1,27 @@
+---
+apiVersion: extensions/v1beta1
+kind: ReplicaSet


Is there a use case when we would have a manifest for ReplicaSet? I thought it's created automatically by Deployment

I haven't encountered a use case, but it is definitely possible to create them separately.

kirs · 2017-06-13T03:54:00Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

-    def fetch_logs
-      @representative_pod ? @representative_pod.fetch_logs : {}
+    def fetch_events
+      return {} unless @latest_rs.present?


I suppose that the deployment would always have a replica set link. Should we raise if it doesn't (catching the possible inconsistent state)?

No, I don't think we should raise. For one, the purpose of fetch_events is collection of debug information, so raising during that would throw a wrench in our attempt to be helpful. For another, since the RS creation happens asynchronously, it is theoretically possible for the RS not to exist (yet) at a given point.

kirs · 2017-06-13T04:00:12Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

+      return unless st.success?
+
+      all_rs_data = JSON.parse(raw_json)["items"]
+      current_revision = deployment_data["metadata"]["annotations"]["deployment.kubernetes.io/revision"]


It may be handy to use deployment_data.dig here (up to you)

That's a Ruby 2.3 feature, so I can't (unless we decide to break with the version supported by shipit-engine, or that gem bumped its requirement and I didn't notice)

kirs · 2017-06-13T04:01:58Z

lib/kubernetes-deploy/kubernetes_resource/pod.rb

        interpret_json_data(pod_data)
      else # reset
        @status = @phase = nil
-        @ready = false
+        @ready = @found = false


Why do we need the @found flag?

It's used by exists?... but I'm not sure why I moved it here if that's what you mean. It should already be set, so I'll change it back.

kirs

Great work on how we can rely on Kubernetes internals and ways to use ownerReference ✨

Most of the things I'd note were already noted by Camilo. I have some minor questions, otherwise LGTM.

On the side note this is one of the best PRs I've seen in terms of referencing the k8s sources and digging into the actual behaviour ❤️ I appreciate the time you spent reading Go sources.

kirs · 2017-06-13T04:19:31Z

test/integration/kubernetes_deploy_test.rb

+    ])
+  end
+
+  def test_can_deploy_deployment_with_zero_replicas


I was going to mention that it's extremely important to make sure that it works with multiple long-running revisions / pods that take more time to shutdown / custom terminationGracePeriod, but it's already tested by test_long_running_deployment and if the test passes I don't have much to add.

KnVerey · 2017-06-13T22:30:42Z

lib/kubernetes-deploy/kubernetes_resource.rb

+          sorted_logs.each do |identifier, log_lines|
+            helpful_info << "  - Logs from container '#{identifier}' (last #{LOG_LINE_COUNT} lines shown):"
+            log_lines.each do |line|
+              helpful_info << "      #{line}"


Now looks like this:

[...event noise addressed by other PR...]

KnVerey · 2017-06-13T22:36:35Z

lib/kubernetes-deploy/kubernetes_resource/pod.rb

      end
+      display_logs if unmanaged? && deploy_succeeded?


Example of this output:

KnVerey · 2017-06-13T23:03:07Z

I made some architectural changes to this PR based on your questions and feedback. Now, whether the resource was created from a deploy template or during a parent resource's sync, its observed state is recorded through its own sync method.interpret_json_data is gone. This way, we have a more unified code path for the resources that can be created both ways (i.e. Pods, ReplicaSets). I'm hoping this will be a lot clearer--lmk what you think!

KnVerey · 2017-06-13T23:09:35Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

+        parent: "#{@name.capitalize} deployment",
+        logger: @logger,
+        deploy_started: @deploy_started
+      ).tap { |rs| rs.sync(latest_rs_data) }


I can also make a method like this if you still think it is better:

def self.from_json(**args) json_data = args.delete(:json_data) inst = new(**args) inst.sync(json_data) inst end

I'm not sure it adds much value, since most of the initializer params aren't from the json, but I'm not against it.

I think is fine as is, what I don't like is tap I find that method to be kinda not very useful sintactic sugar; WDYJ:

rs = RS.new(..) rs.sync

Because we need to return the rs, so it'd be three lines instead of one 😉
Though our style guide doesn't mention it, I've heard other Shopifolk hating on tap in the past. I can change it.

KnVerey · 2017-06-14T20:43:04Z

lib/kubernetes-deploy/kubernetes_resource/service.rb

@@ -16,7 +16,11 @@ def sync
    end

    def deploy_succeeded?
-      @num_endpoints > 0
+      if related_deployment_replicas


The purpose of this change is to be able to handle zero-replica deployments (that have been temporarily scaled down for some reason). I'm second-guessing the what I've done here though--I think it might be too aggressive. If the deployment has rolled out and the service has some endpoints, it's not really valuable to hold up the deploy, is it?

WDTY think about this logic instead:

if exposes_zero_replica_deployment? @num_endpoints == 0 else @num_endpoints > 0 end

I like the explicitness of exposes_zero_replica_deployment? over the current more implicit implementation

camilo

Some more nitpicks, you can choose to ignore (except the one about resource knowing about deploy time)

camilo · 2017-06-14T22:39:31Z

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -192,5 +193,10 @@ def pretty_status
    def kubectl
      @kubectl ||= Kubectl.new(namespace: @namespace, context: @context, logger: @logger, log_failure_by_default: false)
    end
+
+    def template


I like the change; should it be memoized? or would that be roflscale ?

camilo · 2017-06-14T22:41:01Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

-        end
+        deployment_data = JSON.parse(raw_json)
+        @latest_rs = find_latest_rs(deployment_data)
+        @rollout_data = { "replicas" => 0 }.merge!(deployment_data["status"]


dumb nitpick: why prefer the mutating version of merge here? you are using the return value anyway.

no good reason. I refactored this section a bunch of times, so maybe there was some intermediate version that didn't use the return value. I'll fix it.

camilo · 2017-06-14T23:37:14Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

+        parent: "#{@name.capitalize} deployment",
+        logger: @logger,
+        deploy_started: @deploy_started
+      ).tap { |rs| rs.sync(latest_rs_data) }


I think is fine as is, what I don't like is tap I find that method to be kinda not very useful sintactic sugar; WDYJ:

rs = RS.new(..) rs.sync

camilo · 2017-06-14T23:38:07Z

lib/kubernetes-deploy/kubernetes_resource/pod.rb

      @name = name
      @namespace = namespace
      @context = context
      @file = file
      @parent = parent
+      @deploy_started = deploy_started


This is an interesting semantic change here; why would a KubernetesResource be aware of when a deploy started?

It's not actually new. I added it as a parameter to save the parent resource from having to call the setter immediately in these cases. I'll change it back if you think this introduces confusion. See here in the diff for the setter being used instead.

As for why it is needed, it is primarily so that they know when to timeout, although we also use it to filter logs and events. It's not the global deploy start time; it is the time that specific resource was apply'd. For resources in your yamls, the instance is created when we first read in the templates, and this attribute is it is set later, here.

camilo · 2017-06-14T23:39:48Z

lib/kubernetes-deploy/kubernetes_resource/service.rb

@@ -16,7 +16,11 @@ def sync
    end

    def deploy_succeeded?
-      @num_endpoints > 0
+      if related_deployment_replicas


I like the explicitness of exposes_zero_replica_deployment? over the current more implicit implementation

KnVerey · 2017-06-28T01:43:09Z

@camilo @kirs This has been rebased on the refactor I extracted out of it based on your feedback. What's left should look pretty familiar, but could you please take a final look? 😄

KnVerey · 2017-07-04T16:45:17Z

@ibawt can you please review this one?

ibawt · 2017-07-04T18:46:35Z

lib/kubernetes-deploy/kubernetes_resource/replica_set.rb

+    def fetch_events
+      own_events = super
+      return own_events unless @pods.present?
+      own_events.merge(@pods.first.fetch_events)


pods.first: does that mean we'll lose some data or is it the same for the entire thing?

My thinking was that the events will be very similar across the entire set of pods. It is possible some pods will be more interesting than others (e.g. the case you saw where a pod landed on a bad node), but at this stage I don't think we have a nice way of figuring out which one. After my next PR, we could deliberately try to find a failed pod (if any) and use its events... 🤔 Did you have any ideas around this?

no ideas just wanted to make sure I knew the reason.

ibawt

code LGTM

kirs · 2017-06-30T14:14:34Z

lib/kubernetes-deploy/kubernetes_resource/deployment.rb

+
+      latest_rs_data = all_rs_data.find do |rs|
+        rs["metadata"]["ownerReferences"].any? { |ref| ref["uid"] == deployment_data["metadata"]["uid"] } &&
+        rs["metadata"]["annotations"]["deployment.kubernetes.io/revision"] == current_revision


Why do we need to check the revision?

I'm using ownerReferences to be completely certain that the RS belongs to the deployment, and revision to select the new one, as opposed to the n other RS that could exists for this deployment. This seemed like the best available option based one the k8s source and issues I read, but I could be missing something. Are you aware of a better way?

Ah, I somehow thought it's the k8s revision

kirs · 2017-07-04T19:16:07Z

test/integration/kubernetes_deploy_test.rb

+  def test_can_deploy_deployment_with_zero_replicas
+    success = deploy_fixtures("hello-cloud", subset: ["configmap-data.yml", "web.yml.erb"]) do |fixtures|
+      web = fixtures["web.yml.erb"]["Deployment"].first
+      web["spec"]["replicas"] = 0


A use case of zero replicas that I remember is:

Create a deployment with unspecified replicas field

The replicas number is scaled dynamically (can sometimes be zero)

At the moment of the deploy it's zero

I see the test works with YAML that explicitly sets replicas to zero, but would it also support the case of dynamic replicas value set to zero?

Yes, that should work fine. The implementation uses selectors to grab a live copy of the related deployment's template, so it should work even if the deployment wasn't managed by kubernetes-deploy.

👍 great work!

KnVerey force-pushed the replicaset_awareness branch 2 times, most recently from 2fd72e8 to e710be8 Compare June 5, 2017 20:18

KnVerey commented Jun 5, 2017

View reviewed changes

KnVerey requested review from kirs and wfarr June 5, 2017 20:27

KnVerey commented Jun 5, 2017

View reviewed changes

mkobetic reviewed Jun 12, 2017

View reviewed changes

camilo reviewed Jun 13, 2017

View reviewed changes

kirs reviewed Jun 13, 2017

View reviewed changes

KnVerey commented Jun 13, 2017

View reviewed changes

mkobetic approved these changes Jun 14, 2017

View reviewed changes

KnVerey commented Jun 14, 2017

View reviewed changes

KnVerey mentioned this pull request Jun 14, 2017

Log reminders every 30s if nothing new happens #111

Merged

camilo reviewed Jun 14, 2017

View reviewed changes

KnVerey force-pushed the replicaset_awareness branch 2 times, most recently from 94b2e8f to 3d9a5f0 Compare June 21, 2017 19:35

KnVerey mentioned this pull request Jun 21, 2017

Improve handling of KubernetesResource tempfiles/templates #113

Merged

KnVerey force-pushed the replicaset_awareness branch from 78064e9 to c618dda Compare June 28, 2017 01:32

Deployments care about pods in new RS only

8d8ece4

KnVerey force-pushed the replicaset_awareness branch from c618dda to 8d8ece4 Compare June 28, 2017 02:06

ibawt reviewed Jul 4, 2017

View reviewed changes

ibawt approved these changes Jul 4, 2017

View reviewed changes

kirs reviewed Jul 4, 2017

View reviewed changes

KnVerey merged commit 52c2bce into master Jul 4, 2017

KnVerey deleted the replicaset_awareness branch July 4, 2017 19:39

KnVerey had a problem deploying to rubygems July 4, 2017 19:55 Failure

KnVerey deployed to rubygems July 4, 2017 20:19 Active

Replicaset awareness #107

Replicaset awareness #107

Conversation

KnVerey commented Jun 5, 2017 • edited Loading

Problem

Solution

Additional context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Jun 28, 2017

KnVerey commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibawt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KnVerey commented Jun 5, 2017 •

edited

Loading