Don't update the Pod object after each scheduling attempt #73700

denkensk · 2019-02-04T15:00:40Z

What type of PR is this?
/kind bug
/sig scheduling
/priority important-soon

What this PR does / why we need it:
We recently added a PR to put a timestamp on a Pod object every time it is attempted by the scheduler. However, this change could increase the number of pod updates and could raise the load of the API server in some scenarios. This is particularly problematic in clusters with a lot of unschedulable pods that remain unschedulable for a long time and getting attempted many times.

In fact, we don't need to keep this timestamp in the API object. This is needed only for the scheduler to keep track of the time that a pod is attempted. In other words, this timestamp can be kept in the scheduler's internal state only. We should add a timestamp to every pod in the scheduling queue. The timestamp will be the time that the pod is added to the queue. The scheduler should use this timestamp for sorting pods with the same priority. By doing this we will not update the API object after every scheduling attempt.

When I went to solve this issue, I find flush UnschedulableQ Leftover to activeQ in 30s depends on the timestamp the pod is attempted by the scheduler. So In addition to recording the time when pod joined activeq, I also recorded the time when pod joined unschedulerq.

Which issue(s) this PR fixes:
Fixes #73485

Special notes for your reviewer:

Don't update the Pod object after each scheduling attempt by adding a timestamp to the scheduling queue.

denkensk · 2019-02-04T17:37:32Z

/assign @bsalamat

bsalamat

Thanks, @denkensk! Looks good overall. I have some comments about he implementation.

bsalamat · 2019-02-07T19:07:12Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -260,15 +260,34 @@ func podTimestamp(pod *v1.Pod) *metav1.Time {
 	return &condition.LastProbeTime
 }

+// podInfo is minimum cell in the activeQ or the unschedulableQ.


instead of pointing to activeQ or unschedulableQ, just use "scheduling queue" please.

Done thank you.

bsalamat · 2019-02-07T19:07:46Z

pkg/scheduler/internal/queue/scheduling_queue.go

+// podInfo is minimum cell in the activeQ or the unschedulableQ.
+type podInfo struct {
+	pod *v1.Pod
+	// The time pod added to the activeQ or the unschedulableQ.


Same comment about activeQ and unschedulableQ.

Done thank you.

bsalamat · 2019-02-07T19:08:52Z

pkg/scheduler/internal/queue/scheduling_queue.go

+type podInfo struct {
+	pod *v1.Pod
+	// The time pod added to the activeQ or the unschedulableQ.
+	addTime time.Time


I would use queuedTime or just simply timestamp.

Done thank you.

bsalamat · 2019-02-07T19:09:49Z

pkg/scheduler/internal/queue/scheduling_queue.go

+	addTime time.Time
+}
+
+func getPodInfo(pod *v1.Pod, clock util.Clock) *podInfo {


get... gives the impression that this is a getter, but it is not. It is actually a factory that builds a podInfo object. I would rename it to newPodInfo or makePodInfo.

Given how this function is used and the clock that is passed to it, I think it makes sense for the function to be a method of PriorityQueue. That way, you don't need to pass the clock to it every time. You can have a different version of it, for example newPodInfoNoTimestamp or something like that, for the cases that you want to avoid getting the time.

Done Thanks for your suggestions.

bsalamat · 2019-02-07T19:18:33Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

-// that a pod determined as unschedulable multiple times doesn't block any newer pod.
-// This behavior ensures that an unschedulable pod does not block head of the queue when there
-// are frequent events that move pods to the active queue.
-func TestPodFailedSchedulingMultipleTimesDoesNotBlockNewerPod(t *testing.T) {


Why is this test deleted?

Previously, I deleted this test because it was introduced through the previous pr. But now I think this test can be retained. I made some modifications. Thank you.

bsalamat

I missed an important problem in this PR in my first round of reviews. The timestamp of the pod should generally reflect the last time the scheduler has tried scheduling the pod. The time that the pod is added to the queue is a pretty good estimation of the time that the scheduler has tried scheduling the pod. Or, when the pod is recently created, the time it is added to the queue is good estimation of the pod creation time. That's why the time that the pod is added to the scheduling queue is what we are using here.
This time should be recorded when the pod is added to the queue and must not be changed as long as the pod is in the scheduling queue, no matter which substructure of the scheduling queue it moves to. In the current PR, the time is updated every time the pod is added to the activeQ. So, a pod that was tried a long time ago and a pod that is recently tried get almost the same timestamp when they are moved to the activeQ after an event that moves all to the activeQ.

bsalamat · 2019-02-08T22:28:26Z

The timestamp should exists in unschedulable and backoff queues too and shouldn't be changed until the pod is removed from the queue.

denkensk · 2019-02-09T00:50:02Z

Thanks for your review, I also agree that the timestamp shouldn't be changed until the pod is removed from the queue. I will add the timestamp to backoff queues like unschedulableQ. I will implementate that as soon as possible. Thank you again for your help.

denkensk · 2019-02-15T09:34:19Z

Kindly ping @bsalamat PTAL~ thank you~

bsalamat

Thanks, @denkensk.
Your algorithm should add a timestamp to a pod when it is added to the scheduling queue and shouldn't change that timestamp as long as the pod is in the queue, no matter which sub-queue it is being moved to. Please see my comments for more details.

bsalamat · 2019-02-15T21:09:45Z

pkg/scheduler/internal/queue/scheduling_queue.go

 			p.nominatedPods.update(oldPod, newPod)
-			err := p.activeQ.Update(newPod)
+			err := p.activeQ.Update(p.newPodInfo(newPod))


We should keep the pod's existing timestamp here.

Done. I keep the pod's existing timestamp when the pod is updated. Thank you.

bsalamat · 2019-02-15T21:10:10Z

pkg/scheduler/internal/queue/scheduling_queue.go

-			p.podBackoffQ.Delete(newPod)
-			err := p.activeQ.Add(newPod)
+			p.podBackoffQ.Delete(newPodInfoNoTimestamp(oldPod))
+			err := p.activeQ.Add(p.newPodInfo(newPod))


We should keep existing timestamp here too.

bsalamat · 2019-02-15T21:10:19Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -562,7 +576,7 @@ func (p *PriorityQueue) Update(oldPod, newPod *v1.Pod) error {
 			// If the pod is updated reset backoff
 			p.clearPodBackoff(newPod)
 			p.unschedulableQ.delete(usPod)
-			err := p.activeQ.Add(newPod)
+			err := p.activeQ.Add(p.newPodInfo(newPod))


Thanks~ Done

bsalamat · 2019-02-15T21:13:09Z

pkg/scheduler/internal/queue/scheduling_queue.go

+func (p *PriorityQueue) movePodsToActiveQueue(podInfoList []*podInfo) {
+	for _, pInfo := range podInfoList {
+		pod := pInfo.pod
+		if p.isPodBackingOff(pInfo.pod) {


You can use pod instead of pInfo.pod here and below.

Thanks~ Done

denkensk

Thanks, @bsalamat for your review. I addressed your comments because of "force push".

denkensk · 2019-02-16T01:02:22Z

pkg/scheduler/internal/queue/scheduling_queue.go

-			err := p.activeQ.Update(newPod)
+			newPodInfo := p.newPodInfo(newPod)
+			newPodInfo.timestamp = oldPodInfo.(*podInfo).timestamp
+			err := p.activeQ.Update(newPodInfo)


Done. I keep the pod's existing timestamp when the pod is updated. Thank you.

denkensk · 2019-02-16T01:02:34Z

pkg/scheduler/internal/queue/scheduling_queue.go

+			p.podBackoffQ.Delete(newPodInfoNoTimestamp(oldPod))
+			newPodInfo := p.newPodInfo(newPod)
+			newPodInfo.timestamp = oldPodInfo.(*podInfo).timestamp
+			err := p.activeQ.Add(newPodInfo)


denkensk · 2019-02-16T01:03:10Z

pkg/scheduler/internal/queue/scheduling_queue.go

 		if p.isPodBackingOff(pod) {
-			if err := p.podBackoffQ.Add(pod); err != nil {
+			if err := p.podBackoffQ.Add(pInfo); err != nil {


Done. Thank you.

denkensk · 2019-02-16T01:03:27Z

pkg/scheduler/internal/queue/scheduling_queue.go

-			p.unschedulableQ.delete(usPod)
-			err := p.activeQ.Add(newPod)
+			p.unschedulableQ.delete(usPodInfo.pod)
+			err := p.activeQ.Add(usPodInfo)


Done. Thank you.

bsalamat

Thanks, @denkensk. Please see my comments. They are mostly minor comments. I will review the test after they are addressed.

bsalamat · 2019-02-20T23:15:49Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -332,13 +346,13 @@ func (p *PriorityQueue) AddIfNotPresent(pod *v1.Pod) error {
 	if p.unschedulableQ.get(pod) != nil {
 		return nil
 	}
-	if _, exists, _ := p.activeQ.Get(pod); exists {
+	if _, exists, _ := p.activeQ.Get(newPodInfoNoTimestamp(pod)); exists {


Same comment as above

Done thank you

bsalamat · 2019-02-20T23:18:42Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -306,7 +320,7 @@ func (p *PriorityQueue) run() {
 func (p *PriorityQueue) Add(pod *v1.Pod) error {
 	p.lock.Lock()
 	defer p.lock.Unlock()
-	if err := p.activeQ.Add(pod); err != nil {
+	if err := p.activeQ.Add(p.newPodInfo(pod)); err != nil {


In these functions that a podInfo object is needed, please make the object once by calling newPodInfoNoTimestamp or newPodInfo and store it in a local variable and use that variable instead of calling the "constructors" multiple times.

Done thank you

bsalamat · 2019-02-20T23:19:04Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -405,10 +419,10 @@ func (p *PriorityQueue) AddUnschedulableIfNotPresent(pod *v1.Pod, podSchedulingC
 	if p.unschedulableQ.get(pod) != nil {
 		return fmt.Errorf("pod is already present in unschedulableQ")
 	}
-	if _, exists, _ := p.activeQ.Get(pod); exists {
+	if _, exists, _ := p.activeQ.Get(newPodInfoNoTimestamp(pod)); exists {


Done thank you

bsalamat · 2019-02-20T23:53:41Z

pkg/scheduler/internal/queue/scheduling_queue.go

 			p.nominatedPods.update(oldPod, newPod)
-			err := p.activeQ.Update(newPod)
+			newPodInfo := p.newPodInfo(newPod)


Since you are updating the timestamp in the next line, you can use newPodInfoNoTimestamp here.

Done thank you

bsalamat · 2019-02-20T23:54:36Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -537,17 +551,21 @@ func (p *PriorityQueue) Update(oldPod, newPod *v1.Pod) error {

 	if oldPod != nil {
 		// If the pod is already in the active queue, just update it there.
-		if _, exists, _ := p.activeQ.Get(oldPod); exists {
+		if oldPodInfo, exists, _ := p.activeQ.Get(newPodInfoNoTimestamp(oldPod)); exists {


Same as previous comments, please call newPodInfoNoTimestamp(oldPod) once.

Done thank you

bsalamat · 2019-02-20T23:57:01Z

pkg/scheduler/internal/queue/scheduling_queue.go

 			p.nominatedPods.update(oldPod, newPod)
-			p.podBackoffQ.Delete(newPod)
-			err := p.activeQ.Add(newPod)
+			p.podBackoffQ.Delete(newPodInfoNoTimestamp(newPod))


This is not your code, but we should delete oldPod, not newPod, from backoffQ. Given that a Pod name or namespace cannot be updated and we use a pod name and namespace as the key to the map, it shouldn't matter which one of newPod or oldPod we use, but for making the logic clear, we should use oldPod.

Well, yeah, I had some doubts when I first started looking at this code. Change to `oldpod', and the logic becomes clear.

bsalamat · 2019-02-21T00:03:56Z

pkg/scheduler/internal/queue/scheduling_queue.go

-			p.unschedulableQ.delete(usPod)
-			err := p.activeQ.Add(newPod)
+			p.unschedulableQ.delete(usPodInfo.pod)
+			err := p.activeQ.Add(usPodInfo)


This is incorrect. We should create a new podInfo from newPod, update its timestamp with the timestamp of usPodInfo and add the new podInfo to the queue.

Done thank you

bsalamat · 2019-02-21T00:14:42Z

pkg/scheduler/internal/queue/scheduling_queue.go

 }

-// Add adds a pod to the unschedulable pods.
+// Add adds a pod to the unschedulable podInfoMap.
 func (u *UnschedulablePodsMap) addOrUpdate(pod *v1.Pod) {


Please change the argument of this function to a *podInfo instead of a *Pod. That way, you don't need to add a clock to UnschedulablePodsMap. All callers of this function are inside PriorityQueue which can build a podInfo by calling newPodInfo of PriorityQueue.

Done thank you

bsalamat · 2019-02-21T00:15:19Z

pkg/scheduler/internal/queue/scheduling_queue.go

+	// podInfoMap is a map key by a pod's full-name and the value is a pointer to the podInfo.
+	podInfoMap map[string]*podInfo
+	keyFunc    func(*v1.Pod) string
+	clock      util.Clock


See my comment on addOrUpdate below. I don't think we need a clock here.

Done thank you

bsalamat

Thanks, @denkensk. Code looks good. Test needs improvement.

bsalamat · 2019-02-21T01:33:24Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

 }

 func getUnschedulablePod(p *PriorityQueue, pod *v1.Pod) *v1.Pod {
 	p.lock.Lock()
 	defer p.lock.Unlock()
-	return p.unschedulableQ.get(pod)
+	if p.unschedulableQ.get(pod) != nil {


Please use:

Suggested change

if p.unschedulableQ.get(pod) != nil {

pInfo := p.unschedulableQ.get(pod)

if pInfo != nil {

return pInfo.pod

}

Done thank you

bsalamat · 2019-02-21T01:37:13Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

-		Status:        v1.ConditionFalse,
-		Reason:        v1.PodReasonUnschedulable,
-		Message:       "fake scheduling failure",
-		LastProbeTime: metav1.Now(),


You can remove LastProbeTime here, but please keep the rest of the code. Otherwise, the scheduling queue will not consider the pod as "unschedulable".

Done thank you

bsalamat · 2019-02-21T01:37:38Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -841,15 +836,6 @@ func TestPodFailedSchedulingMultipleTimesDoesNotBlockNewerPod(t *testing.T) {
 		},
 	}
 	q.Add(&newerPod)
-
-	// And then unschedulablePod was determined as unschedulable AGAIN.
-	podutil.UpdatePodCondition(&unschedulablePod.Status, &v1.PodCondition{


same comment here and below.

Done thank you

bsalamat · 2019-02-21T01:39:44Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -990,3 +960,66 @@ func TestHighProirotyFlushUnschedulableQLeftover(t *testing.T) {
 		t.Errorf("Expected: %v after Pop, but got: %v", medPriorityPod.Name, p.Name)
 	}
 }
+
+// TestPodInfoInActiveQ tests that the pods with the same priority is sorted by the podInfo.timestamp.


Done thank you

bsalamat · 2019-02-21T01:44:34Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -990,3 +960,66 @@ func TestHighProirotyFlushUnschedulableQLeftover(t *testing.T) {
 		t.Errorf("Expected: %v after Pop, but got: %v", medPriorityPod.Name, p.Name)
 	}
 }
+
+// TestPodInfoInActiveQ tests that the pods with the same priority is sorted by the podInfo.timestamp.
+func TestPodInfoInActiveQComp(t *testing.T) {


We need to test many more scenarios in this test, such as updating a pod in the activeQ, adding unschedulabe pod to the unschelableQ and then triggering a move request to the activeQ and ensuring that the timestamp is not updated, another similar test to check that when a pod goes to backoffQ and then the activeQ its timestamp remains the same, ....
You may want to take a look at this test to get ideas about how you can add many test scenarios involving various operations in this test.

Thanks for your help. I add many more scenarios in this test about activeQ/unschedulabeQ/backoffQ. If there's anything else missing, you can comment and I'll add it as soon as possible.

bsalamat

Thanks, @denkensk. Looks good. Just a few minor comments.

bsalamat · 2019-02-21T23:09:38Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+		expected   []*podInfo
+	}{
+		{
+			name: "add two pod to activeQ and pod them by the timestamp",


s/pod them/sort them/

Done thank you

bsalamat · 2019-02-21T23:11:42Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -990,3 +979,135 @@ func TestHighProirotyFlushUnschedulableQLeftover(t *testing.T) {
 		t.Errorf("Expected: %v after Pop, but got: %v", medPriorityPod.Name, p.Name)
 	}
 }
+
+// TestHighProirotyPodInfo tests the operations related to podInfo.
+func TestHighProirotyPodInfo(t *testing.T) {


I would rename this to something like TestPodTimestamp or something like that. In this test we don't care much about priority. So, having "HighPriority" in the name of the test is not necessary.

Done thank you

bsalamat · 2019-02-21T23:15:34Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+		{
+			name: "add one pod to BackoffQ and move it to activeQ",
+			operations: []operation{
+				addPodBackoffQ(pInfo1), flushBackoffQ(), moveAllToActiveQ(),


Could you add pInfo2 to the activeQ in this test case and check that pInfo2 is after pInfo1 in the expected output?

Done thank you

bsalamat · 2019-02-21T23:40:19Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

-	addOrUpdateUnschedulablePod(q, &midPod)
-
-	// Update pod condition to highPod.
-	podutil.UpdatePodCondition(&highPod.Status, &v1.PodCondition{


Please keep these status updates and move them above the lines that add pods to the unschedulableQ.

Done thank you

denkensk · 2019-02-22T03:54:10Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+		return func() {
+			queue.lock.Lock()
+			defer queue.lock.Unlock()
+			queue.podBackoffQ.Add(pInfo)


@bsalamat thank you~ Here I add comments to facilitate your review. Add lock upper and here to prevent data race occuring in https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/73700/pull-kubernetes-bazel-test/76841/

bsalamat

Thanks a lot, @denkensk!

/lgtm
/approve

k8s-ci-robot · 2019-02-22T04:39:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, denkensk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [bsalamat]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

denkensk · 2019-02-22T17:37:54Z

Thank you very much for your patience and your help. @bsalamat

liggitt · 2019-02-26T16:35:47Z

looks like this introduced a flake in unit tests:

first occurrence: https://gubernator.k8s.io/build/kubernetes-jenkins/pr-logs/pull/73700/pull-kubernetes-bazel-test/76761#pkgschedulerinternalqueuego_default_test

https://storage.googleapis.com/k8s-gubernator/triage/index.html?pr=1&text=Unable%20to%20find%20backoff%20value

denkensk · 2019-03-01T06:08:52Z

@liggitt Thanks, the flake in unit tests fixed in this pr #74611 today.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2019

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from a47a81a to bc5fe08 Compare February 4, 2019 15:08

denkensk changed the title ~~Don't update the Pod object after each scheduling attempt~~ [WIP]Don't update the Pod object after each scheduling attempt Feb 4, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2019

denkensk changed the title ~~[WIP]Don't update the Pod object after each scheduling attempt~~ Don't update the Pod object after each scheduling attempt Feb 4, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2019

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from bc5fe08 to 4495fe9 Compare February 4, 2019 16:30

k8s-ci-robot assigned bsalamat Feb 4, 2019

bsalamat reviewed Feb 7, 2019

View reviewed changes

bsalamat reviewed Feb 8, 2019

View reviewed changes

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from cd511eb to 0039575 Compare February 15, 2019 07:21

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 15, 2019

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from 0039575 to db3b8ef Compare February 15, 2019 07:32

bsalamat reviewed Feb 15, 2019

View reviewed changes

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from db3b8ef to ebdbb57 Compare February 16, 2019 00:53

denkensk commented Feb 16, 2019

View reviewed changes

bsalamat reviewed Feb 21, 2019

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2019

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from 664e678 to fa8c3df Compare February 21, 2019 16:59

bsalamat reviewed Feb 21, 2019

View reviewed changes

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from fa8c3df to 0b945bc Compare February 22, 2019 00:46

not updae timestamp for each scheduling attempt

ea9e1a4

denkensk force-pushed the no-updae-timestamp-each-scheduling-attempt branch from 0b945bc to ea9e1a4 Compare February 22, 2019 01:46

denkensk commented Feb 22, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2019

bsalamat approved these changes Feb 22, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2019

k8s-ci-robot merged commit bf3b5e5 into kubernetes:master Feb 22, 2019

bsalamat mentioned this pull request Feb 22, 2019

Scheduler doesn't schedule any pods for short periods of time #73176

Closed

denkensk mentioned this pull request Feb 26, 2019

fix the flake in scheduling_queue_test #74611

Merged

denkensk mentioned this pull request Mar 27, 2019

clean up func podTimestamp in queue #75754

Merged

yqwang-ms mentioned this pull request Oct 12, 2019

Should use Pod creation time instead of its last queued time to sort Pod for the same priority #83826

Closed

-	if p.unschedulableQ.get(pod) != nil {
+	pInfo := p.unschedulableQ.get(pod)
+	if pInfo != nil {
+		return pInfo.pod
+	}

Don't update the Pod object after each scheduling attempt #73700

Don't update the Pod object after each scheduling attempt #73700

Conversation

denkensk commented Feb 4, 2019 • edited

denkensk commented Feb 4, 2019

bsalamat 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

bsalamat left a comment

Choose a reason for hiding this comment

bsalamat commented Feb 8, 2019

denkensk commented Feb 9, 2019 • edited

denkensk commented Feb 15, 2019

bsalamat 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

denkensk 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

bsalamat 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

denkensk Feb 21, 2019 • edited

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

bsalamat 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

bsalamat 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

denkensk commented Feb 4, 2019 •

edited

denkensk commented Feb 9, 2019 •

edited

denkensk Feb 21, 2019 •

edited