StatefulSet kubectl rollout command #49674

crimsonfaith91 · 2017-07-27T00:48:57Z

What this PR does / why we need it: This PR implements StatefulSet kubectl rollout command, covering history, status, and undo.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49890

Special notes for your reviewer:

Release note:

kubectl rollout `history`, `status`, and `undo` subcommands now support StatefulSets.

crimsonfaith91 · 2017-08-03T23:35:24Z

@kow3ns @foxish @janetkuo
I am testing the kubectl rollout command via hack/make-rules/test-cmd.sh, but was stuck for a few days on testing existence of controllerrevision in metadata.annotations. The error message below indicates that controllerrevision cannot be found via metadata.annotations:

Get controllerrevisions {{range.items}}{{.metadata.annotations}}:{{end}}
  Expected: .*rollingupdate-statefulset-rv2.yaml --record.*
  Got:      <no value>:<no value>:

I am not sure whether statefulset handles controllerrevision similarly as daemonset. I notice that the test was not originally added in daemonset rollout command PR. It was added in another PR afterwards.

Do you have any idea why adding --record flag does not store controllerrevision info in statefulset metadata.annotations? The YAML file that creates statefulset for testing purpose can be found here. Thanks!

foxish · 2017-08-04T21:14:35Z

cc @kubernetes/sig-cli-pr-reviews

dims · 2017-08-07T20:34:50Z

hack/testdata/rollingupdate-statefulset.yaml

      containers:
      - name: nginx
-        image: gcr.io/google_containers/nginx-slim:0.7
+        image: gcr.io/google-containers/pause:latest


image does not match the name, right?

yup, i will change the container name.

dims · 2017-08-07T20:36:47Z

pkg/controller/statefulset/stateful_set.go

@@ -461,3 +477,137 @@ func (ssc *StatefulSetController) syncStatefulSet(set *apps.StatefulSet, pods []
 	glog.V(4).Infof("Successfully synced StatefulSet %s/%s successful", set.Namespace, set.Name)
 	return nil
 }
+
+/*


Do we need this commented out code?

It looks like i have removed the codes from the newest commit.

dims · 2017-08-07T20:37:40Z

staging/src/k8s.io/client-go/listers/apps/v1beta1/statefulset_expansion.go

@@ -75,3 +76,39 @@ func (s *statefulSetLister) GetPodStatefulSets(pod *v1.Pod) ([]*apps.StatefulSet

 	return psList, nil
 }
+
+/*


Commented out? needed?

Not needed - i will remove it.

i have removed it from the newest commit, so no change is needed.

mengqiy

Kubectl change and cmd test are mostly LGTM

mengqiy · 2017-08-18T00:37:09Z

pkg/kubectl/history.go

 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to retrieve DaemonSet %s: %v", name, err)
+		return nil, nil, fmt.Errorf("failed to obtain accessor for %s named %s: %v", kind, accessor.GetName(), err)


accessor.GetName()

Why not simply use name? It should be identical.

ya, name suffices. :)

mengqiy · 2017-08-18T00:37:45Z

pkg/kubectl/history.go

 	if err != nil {
 		return nil, nil, err
 	}
-	historyList, err := apps.ControllerRevisions(ds.Namespace).List(metav1.ListOptions{LabelSelector: selector.String()})
+	historyList, err := apps.ControllerRevisions(accessor.GetNamespace()).List(metav1.ListOptions{LabelSelector: selector.String()})


accessor.GetNamespace()

Same question. Why not using namespace?

Looking at the original codes, namespace may be different from accessor.GetNamespace().

I will verify whether they are the same, and remove Accessor codes if so.

I think they should be the same, since the logic chain is
parameters<name, namespace> -> obj -> accessor -> GetNamespace()

Noted! Thanks for confirming - let me remove the Accessor codes.

mengqiy · 2017-08-18T01:31:35Z

hack/make-rules/test-cmd-util.sh

@@ -3627,7 +3680,7 @@ run_stateful_set_tests() {
  # Pre-condition: no statefulset exists
  kube::test::get_object_assert statefulset "{{range.items}}{{$id_field}}:{{end}}" ''
  # Command: create statefulset
-  kubectl create -f hack/testdata/nginx-statefulset.yaml "${kube_flags[@]}"
+  kubectl create -f hack/testdata/rollingupdate-statefulset.yaml "${kube_flags[@]}"


Just curious why changing this?
If hack/testdata/nginx-statefulset.yaml is no longer needed, please delete it.

I initally planned to reuse nginx-statefulset.yaml, but found out that updateStrategy of the spec has to be changed. I am not sure whether it is fine to change existing YAML test files, so i created a new one.

I will remove nginx-statefulset.yaml if it is not used at other places.

mengqiy · 2017-08-18T01:32:02Z

hack/make-rules/test-cmd-util.sh

-
+  image_field0="(index .spec.template.spec.containers 0).image"
+  image_field1="(index .spec.template.spec.containers 1).image"
+  


nit: remove white space

mengqiy · 2017-08-18T01:32:52Z

hack/testdata/rollingupdate-statefulset-rv2.yaml

+        - -c
+        - 'while true; do sleep 1; done'
+
+


nit: remove unnecessary white space and newline.

yup! i will remove it - it is an issue with my text editor.

mengqiy · 2017-08-18T01:32:58Z

hack/testdata/rollingupdate-statefulset.yaml

+        - -c
+        - 'while true; do sleep 1; done'
+
+


nit: remove unnecessary white space and newline.

crimsonfaith91 · 2017-08-18T20:51:19Z

@mengqiy Addressed your feedback. Thanks! :)

janetkuo · 2017-08-21T17:46:21Z

Do you have any idea why adding --record flag does not store controllerrevision info in statefulset metadata.annotations? The YAML file that creates statefulset for testing purpose can be found here. Thanks!

Regarding --record it probably has something to do with how kubectl apply handles --record (e.g. apply the patch first and then apply the recorded commands separately). I'll take a look.

janetkuo · 2017-08-21T18:03:24Z

hack/testdata/rollingupdate-statefulset.yaml

@@ -0,0 +1,25 @@
+apiVersion: apps/v1beta1


Use apps/v1beta2 instead so that we make sure apps/v1beta2 StatefulSet works?

Agreed. It makes sense to use apps/v1beta2 as we are migrating to it.

Would you change the rollingupdate-daemonset yaml files (under testdata) to apps/v1beta2 as well while you're at it?

yup! we should also do so for deployment.

Would you change the rollingupdate-daemonset yaml files (under testdata) to apps/v1beta2 as well while you're at it?

Actually, we can do this separately in another PR. I'll file a PR for this, you just need to modify the statefulset manifest to use apps/v1beta2

Ya, let's do it in another PR.

I filed #51061 for manifests other than StatefulSet

janetkuo · 2017-08-21T18:09:47Z

hack/make-rules/test-cmd-util.sh

-  daemonset_image_field0="(index .spec.template.spec.containers 0).image"
-  daemonset_image_field1="(index .spec.template.spec.containers 1).image"
+  image_field0="(index .spec.template.spec.containers 0).image"
+  image_field1="(index .spec.template.spec.containers 1).image"


We can replace deployment_image_field and deployment_second_image_field with these two as well

janetkuo · 2017-08-21T18:14:40Z

hack/testdata/rollingupdate-statefulset.yaml

+      terminationGracePeriodSeconds: 5
+      containers:
+      - name: kubernetes-pause
+        image: gcr.io/google-containers/pause:latest


Use gcr.io/google_containers/nginx-slim:0.7 for this and gcr.io/google_containers/nginx-slim:0.8 for rv2 (since the name is nginx)

janetkuo · 2017-08-21T18:17:03Z

hack/make-rules/test-cmd-util.sh

+  # Update the statefulset (revision 2)
+  kubectl apply -f hack/testdata/rollingupdate-statefulset-rv2.yaml --record "${kube_flags[@]}"
+  kube::test::wait_object_assert statefulset "{{range.items}}{{$image_field0}}:{{end}}" "${IMAGE_STATEFULSET_R2}:"
+  kube::test::get_object_assert statefulset "{{range.items}}{{$container_len}}{{end}}" "1"


In DaemonSet history test I put 1 container in v1 and 2 containers in v2 intentionally, so that I can test if the apply and rollback work. Then this part is to make sure the number of containers is correct. Would you change it accordingly?

I didn't manage to get 2 containers into the statefulset YAML file a few weeks ago. Let me try again. :)

janetkuo · 2017-08-21T18:29:42Z

pkg/kubectl/history.go

 	versionedAppsClient := versionedAppsClientV1beta1(h.c)
-	ds, allHistory, err := controlledHistories(versionedExtensionsClient, versionedAppsClient, namespace, name)
+	versionedExtensionsClient := versionedExtensionsClientV1beta1(h.c)


This change isn't necessary (unless there's a reason to move it?)

I switched the order that puts versionedExtensionsClient before versionedAppsClient. There are 2 reasons behind:
(1) it is alphabetically ordered for having versionedAppsClient before versionedExtensionsClient
(2) controlledHistories function brings argument in the order of versionedAppsClient and then versionedExtensionsClient

janetkuo · 2017-08-21T18:36:11Z

pkg/kubectl/history.go

+			return nil, nil, fmt.Errorf("failed to retrieve StatefulSet %s: %v", name, err)
+		}
+		labelSelector = ss.Spec.Selector
+		obj = ss


Should we add a default case and return error?

janetkuo · 2017-08-21T19:54:55Z

pkg/kubectl/rollback.go

-	versionedDS, allHistory, err := controlledHistories(versionedExtensionsClient, versionedAppsClient, ds.Namespace, ds.Name)
+	versionedExtensionsClient := versionedExtensionsClientV1beta1(r.c)
+	versionedObj, allHistory, err := controlledHistories(versionedAppsClient, versionedExtensionsClient, ds.Namespace, ds.Name, "DaemonSet")
+	versionedDS := versionedObj.(*externalextensions.DaemonSet)
 	if err != nil {


error check should be done immediately after where err is returned

make sense!

janetkuo · 2017-08-21T19:57:43Z

pkg/kubectl/rollback.go

-	versionedDS, allHistory, err := controlledHistories(versionedExtensionsClient, versionedAppsClient, ds.Namespace, ds.Name)
+	versionedExtensionsClient := versionedExtensionsClientV1beta1(r.c)
+	versionedObj, allHistory, err := controlledHistories(versionedAppsClient, versionedExtensionsClient, ds.Namespace, ds.Name, "DaemonSet")
+	versionedDS := versionedObj.(*externalextensions.DaemonSet)


This will panic if versionedObj is not a *externalextensions.DaemonSet. It's probably okay, but it'll be safer to do something like:

versionedDS, ok := versionedObj.(*externalextensions.DaemonSet) // return an error if !ok

Same comment for the other type assertion in this PR

Noted! Thanks for pointing it out - this is very important as i use type assertion frequently.

janetkuo · 2017-08-21T20:02:04Z

pkg/kubectl/rollback.go

+}
+
+// toRevision is a non-negative integer, with 0 being reserved to indicate rolling back to previous configuration
+func (r *StatefulSetRollbacker) Rollback(obj runtime.Object, updatedAnnotations map[string]string, toRevision int64, dryRun bool) (string, error) {


Lots of code duplication with (r *DaemonSetRollbacker) Rollback. Would you de-duplicate it (or at least part of it)?

Sure! i will carry out the de-duplication using polymorphism runtime.Object.

janetkuo · 2017-08-21T22:31:51Z

@crimsonfaith91 there's a bug in StatefulSet update validation which caused changing the number of containers impossible for StatefulSets. I filed a patch to fix it: #51051

@smarterclayton

Automatic merge from submit-queue (batch tested with PRs 50980, 46902, 51051, 51062, 51020) Fix StatefulSet update validation StatefulSet update validation did not allow change to number of containers in pod template. Fix this bug so that it's possible to make this kind of change. Found it when suggesting test-cmd changes in #49674. @kubernetes/sig-apps-pr-reviews @smarterclayton /approve no-issue

janetkuo · 2017-08-23T23:46:15Z

hack/testdata/rollingupdate-statefulset-rv2.yaml

@@ -39,3 +26,12 @@ spec:
        - sh
        - -c
        - 'while true; do sleep 1; done'
+      - name: nginx-2
+        image: gcr.io/google_containers/nginx-slim:0.8


Would you update nginx to gcr.io/google_containers/nginx-slim:0.8 and add another container (e.g. gcr.io/google-containers/pause:2.0).

janetkuo · 2017-08-23T23:59:14Z

hack/make-rules/test-cmd-util.sh

+  kube::test::get_object_assert statefulset "{{range.items}}{{$id_field}}:{{end}}" ''
+
+  # Command: create a StatefulSet (revision 1)
+  kubectl apply -f hack/testdata/rollingupdate-statefulset.yaml --record "${kube_flags[@]}"


I tried --record locally and it did work. We need to verify controllerrevisions' annotations here first, otherwise this test might be flaky (when undo is called before StS controller creates controllerrevisions)

crimsonfaith91 · 2017-08-24T00:31:00Z

hack/make-rules/test-cmd-util.sh

@@ -3007,10 +3007,10 @@ run_daemonset_history_tests() {
  # Command
  # Create a DaemonSet (revision 1)
  kubectl apply -f hack/testdata/rollingupdate-daemonset.yaml --record "${kube_flags[@]}"
-  kube::test::get_object_assert controllerrevisions "{{range.items}}{{$annotations_field}}:{{end}}" ".*rollingupdate-daemonset.yaml --record.*"
+  kube::test::wait_object_assert controllerrevisions "{{range.items}}{{$annotations_field}}:{{end}}" ".*rollingupdate-daemonset.yaml --record.*"


@janetkuo We have to change get_object_assert to wait_object_assert here. :)

janetkuo · 2017-08-24T00:37:15Z

hack/testdata/rollingupdate-statefulset-rv2.yaml

+        command:
+        - sh
+        - -c
+        - 'while true; do sleep 1; done'


You can't run these commands in pause. Just remove them.

janetkuo · 2017-08-24T00:38:11Z

hack/testdata/rollingupdate-statefulset-rv2.yaml

+        - sh
+        - -c
+        - 'while true; do sleep 1; done'
+      - name: nginx-2


name it pause instead

janetkuo · 2017-08-24T18:40:03Z

hack/make-rules/test-cmd-util.sh

+  kube::test::get_object_assert statefulset "{{range.items}}{{$container_len}}{{end}}" "2"
+  # Clean up - delete newest configuration
+  kubectl delete -f hack/testdata/rollingupdate-statefulset-rv2.yaml "${kube_flags[@]}"
+   # Post-condition: no pods from statefulset controller


Remove the redundant space in the beginning

janetkuo · 2017-08-24T19:44:11Z

pkg/kubectl/history.go

@@ -175,7 +175,7 @@ func (h *DaemonSetHistoryViewer) ViewHistory(namespace, name string, revision in
 		if !ok {
 			return "", fmt.Errorf("unable to find the specified revision")
 		}
-		dsOfHistory, err := applyHistory(ds, history)
+		dsOfHistory, err := applyHistory(versionedObj.(*extensionsv1beta1.DaemonSet), history)


Add the check I suggested to make sure it won't panic

i will check to ensure i didn't miss this in other places.

janetkuo · 2017-08-24T19:46:44Z

pkg/kubectl/history.go

+		labelSelector = ss.Spec.Selector
+		obj = ss
+	default:
+		return nil, nil, fmt.Errorf("undefined API object kind: %s", kind)


"unsupported", not undefined

Ya, as the switch case is not exhaustive, unsupported is a better word.

janetkuo · 2017-08-24T20:02:50Z

pkg/kubectl/rollback.go

 	if err != nil {
 		return "", fmt.Errorf("unable to find history controlled by DaemonSet %s: %v", ds.Name, err)
 	}
+	versionedDS, ok := versionedObj.(*externalextensions.DaemonSet)
+	if !ok {
+		return "", fmt.Errorf("unable to perform type assertion for DaemonSet %s", ds.Name)


If !ok, it means that the value doesn't hold the type. So the error message should say that the thing is not of DaemonSet type. Something like "unexpected non-DaemonSet object returned: %v", versionedDS.

Same for other type assertion error messages.

janetkuo · 2017-08-24T20:10:40Z

pkg/kubectl/rollback.go

+			if h.Revision == toRevision {
+				// If toRevision != 0, find the history with matching revision
+				toHistory = h
+				break


Since this is extracted into a function, you can replace the above 2 lines with return h

janetkuo · 2017-08-24T20:13:16Z

pkg/kubectl/rollback.go

+	if toRevision == 0 {
+		// If toRevision == 0, find the latest revision (2nd max)
+		sort.Sort(historiesByRevision(allHistory))
+		toHistory = allHistory[len(allHistory)-2]


Since this is now a function, you should return an error when len(allHistory) <= 1. The callers already checked it, but it's safer to check it again, so that future callers won't cause a panic (index out of range).

For len(allHistory) == 0, we should return nil.
For len(allHistory) == 1, we should return allHistory[0].

No, you have to return nil for both cases. findHistory finds the history of a specific revision number. If there's only one history (let's say it's rv 5), and I asked for rv 10, you should return me nil instead of blindly return me whatever history you have.

Please add godoc for findHistory as well.

Or you can check if allHistory[0].Revision == toRevision when the length is 1.

i thought about this, but the original check is shorter:

if toRevision == 0 && len(allHistory) <= 1 {

janetkuo · 2017-08-24T20:22:23Z

pkg/kubectl/rollback.go

+	return toHistory
+}
+
+func isPodTemplateConvertible(specTemplate *v1.PodTemplateSpec) (string, error) {


The function name is misleading. This function doesn't check if the given template is convertible, but converts and prints the v1 template in api template format. Name it "printTemplate" instead.

Will printPodTemplate be better?

janetkuo · 2017-08-24T23:33:42Z

pkg/kubectl/rollback.go

+}
+
+// findHistory checks whether controller revision exists for rollback purpose
+func findHistory(toRevision int64, allHistory []*appsv1beta1.ControllerRevision) *appsv1beta1.ControllerRevision {


An example godoc:

// findHistory returns a controllerrevision of a specific revision from the given controllerrevisions. // It returns nil if no such controllerrevision exists. // If toRevision is 0, the last previously used history is returned.

janetkuo · 2017-08-24T23:37:25Z

pkg/kubectl/rollback.go

+}
+
+// printPodTemplate converts pod template to unversioned before printing out the template
+func printPodTemplate(specTemplate *v1.PodTemplateSpec) (string, error) {


// printPodTemplate converts a given pod template into a human-readable string

We convert it first because DescribePodTemplate only accepts internal representation of a pod template. Converting the template isn't the main thing this function does, but printing it is.

I will change the godoc messages accordingly.

k8s-ci-robot · 2017-08-24T23:38:52Z

@crimsonfaith91: you cannot LGTM your own PR.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

janetkuo · 2017-08-24T23:48:43Z

/lgtm

spxtr · 2017-08-24T23:59:15Z

hack/make-rules/test-cmd-util.sh

@@ -3041,6 +3044,58 @@ run_daemonset_history_tests() {
  set +o errexit
 }

+run_statefulset_history_tests() {
+  set -o nounset


Leave this set, and if you want to not fail on certain calls them put them in an "if"

i follow the template of run_daemonset_history_tests: https://github.com/kubernetes/kubernetes/blob/master/hack/make-rules/test-cmd-util.sh#L2998

If we are to remove this line for statefulset, we should also do so for daemonset.

spxtr · 2017-08-25T00:17:52Z

/approve no-issue

k8s-github-robot · 2017-08-25T00:18:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crimsonfaith91, janetkuo, spxtr

Associated issue: 49890

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

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [spxtr]
~~pkg/controller/statefulset/OWNERS~~ [janetkuo]
~~pkg/kubectl/OWNERS~~ [janetkuo]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-08-25T18:07:13Z

Automatic merge from submit-queue (batch tested with PRs 50033, 49988, 51132, 49674, 51207)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 27, 2017

k8s-github-robot assigned mengqiy and eparis Jul 27, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 27, 2017

crimsonfaith91 force-pushed the rollout branch from 877f6b6 to 6a0934b Compare July 27, 2017 17:07

crimsonfaith91 mentioned this pull request Jul 27, 2017

Failed tests: pull-kubernetes-e2e-gce-etcd3 #49672

Closed

crimsonfaith91 changed the title ~~[WIP] StatefulSet kubectl rollout command~~ StatefulSet kubectl rollout command Jul 27, 2017

k8s-ci-robot assigned janetkuo and unassigned eparis Jul 27, 2017

k8s-github-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 Jul 27, 2017

crimsonfaith91 changed the title ~~StatefulSet kubectl rollout command~~ [WIP] StatefulSet kubectl rollout command Jul 27, 2017

crimsonfaith91 force-pushed the rollout branch 2 times, most recently from e4d5bad to c00edea Compare August 1, 2017 22:48

crimsonfaith91 changed the title ~~[WIP] StatefulSet kubectl rollout command~~ StatefulSet kubectl rollout command Aug 3, 2017

crimsonfaith91 force-pushed the rollout branch from c00edea to f5cfd8b Compare August 3, 2017 02:19

crimsonfaith91 force-pushed the rollout branch 2 times, most recently from bc999df to 99fb2e8 Compare August 3, 2017 23:54

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 3, 2017

crimsonfaith91 force-pushed the rollout branch from 99fb2e8 to 5ebf34f Compare August 3, 2017 23:55

k8s-ci-robot unassigned mengqiy Aug 3, 2017

crimsonfaith91 force-pushed the rollout branch from 5ebf34f to 98bee09 Compare August 4, 2017 00:46

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Aug 4, 2017

dims reviewed Aug 7, 2017

View reviewed changes

mengqiy reviewed Aug 18, 2017

View reviewed changes

crimsonfaith91 force-pushed the rollout branch 2 times, most recently from 198217f to 0bbd6dc Compare August 18, 2017 20:50

janetkuo reviewed Aug 21, 2017

View reviewed changes

janetkuo mentioned this pull request Aug 21, 2017

Fix StatefulSet update validation #51051

Merged

crimsonfaith91 force-pushed the rollout branch 2 times, most recently from d926e1b to a7b51ac Compare August 23, 2017 17:45

janetkuo reviewed Aug 23, 2017

View reviewed changes

crimsonfaith91 commented Aug 24, 2017

View reviewed changes

janetkuo reviewed Aug 24, 2017

View reviewed changes

statefulSet kubectl rollout command

ebdbafd

crimsonfaith91 force-pushed the rollout branch from 4cbfab4 to ebdbafd Compare August 24, 2017 23:43

janetkuo approved these changes Aug 24, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2017

spxtr reviewed Aug 24, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2017

k8s-github-robot merged commit c19785c into kubernetes:master Aug 25, 2017

crimsonfaith91 deleted the rollout branch August 25, 2017 18:08


		image_field0="(index .spec.template.spec.containers 0).image"
		image_field1="(index .spec.template.spec.containers 1).image"

StatefulSet kubectl rollout command #49674

StatefulSet kubectl rollout command #49674

Conversation

crimsonfaith91 commented Jul 27, 2017 • edited Loading

crimsonfaith91 commented Aug 3, 2017 • edited Loading

foxish commented Aug 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy 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

crimsonfaith91 Aug 18, 2017 • edited Loading

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

crimsonfaith91 commented Aug 18, 2017

janetkuo commented Aug 21, 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

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

janetkuo commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo Aug 23, 2017 • edited Loading

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

janetkuo Aug 24, 2017 • edited Loading

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

crimsonfaith91 commented Jul 27, 2017 •

edited

Loading

crimsonfaith91 commented Aug 3, 2017 •

edited

Loading

crimsonfaith91 Aug 18, 2017 •

edited

Loading

janetkuo Aug 23, 2017 •

edited

Loading

janetkuo Aug 24, 2017 •

edited

Loading