controller: Refactor controller_pod #548

gaocegege · 2018-04-20T11:21:24Z

This change is

coveralls · 2018-04-20T11:38:18Z

Coverage decreased (-1.0%) to 49.4% when pulling d83e7fb on gaocegege:slice into 60d25c4 on kubeflow:master.

gaocegege · 2018-04-24T03:46:32Z

/assign @ScorpioCPH @jlewi

ScorpioCPH

Thanks, I like this function getPodSlices() :)

ScorpioCPH · 2018-04-24T03:48:39Z

pkg/controller.v2/controller_pod.go

 }

 func getPodSlices(pods []*v1.Pod, replicas int, logger *log.Entry) [][]*v1.Pod {
-	podSlices := make([][]*v1.Pod, 0)
+	logger.Infof("%v", pods)


This log maybe too verbose.

ScorpioCPH · 2018-04-24T03:51:29Z

pkg/controller.v2/controller_pod.go

@@ -212,9 +198,9 @@ func getPodSlices(pods []*v1.Pod, replicas int, logger *log.Entry) [][]*v1.Pod {
 		}


Should we break here?

Sorry, I rebased the master and can not see it anymore, could you have another review and point it out?

It has already been addressed.

ScorpioCPH · 2018-04-24T03:53:33Z

pkg/controller.v2/controller_pod.go

 }

 func getPodSlices(pods []*v1.Pod, replicas int, logger *log.Entry) [][]*v1.Pod {
-	podSlices := make([][]*v1.Pod, 0)
+	logger.Infof("%v", pods)
+	podSlices := make([][]*v1.Pod, replicas)


How about use a map for podSlices, like this:

podSlices := make(map[int64][]*v1.Pod, replicas)

ScorpioCPH · 2018-04-24T03:56:53Z

pkg/controller.v2/controller_pod.go

 }

 func getPodSlices(pods []*v1.Pod, replicas int, logger *log.Entry) [][]*v1.Pod {
-	podSlices := make([][]*v1.Pod, 0)
+	logger.Infof("%v", pods)
+	podSlices := make([][]*v1.Pod, replicas)
 	for _, pod := range pods {
 		if _, ok := pod.Labels[tfReplicaIndexLabel]; !ok {
 			logger.Warning("The pod do not have the index label.")


continue here?

ScorpioCPH · 2018-04-24T03:57:13Z

pkg/controller.v2/controller_pod.go

-		// TODO(CPH): Need to delete pods.
-		loggerForTFJob(tfjob).Infof("need to delete pod but it is not implemented yet")
+		// We already have one, and check if it is succeede or something else.
+		// pod := podSlice[0]


What this line used for?

Signed-off-by: Ce Gao <gaoce@caicloud.io>

gaocegege · 2018-04-24T04:20:42Z

Addressed.

Signed-off-by: Ce Gao <gaoce@caicloud.io>

jlewi · 2018-04-24T17:24:41Z

nit; it would be great if you could add a more descriptive PR description. If its not making substantive changes I'm happy to leave it to you @ScorpioCPH

jlewi · 2018-04-24T17:36:51Z

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

pkg/controller.v2/controller_pod.go, line 189 at r2 (raw file):

Previously, ScorpioCPH (Penghao Cen) wrote…

This log maybe too verbose.

IME we should log sufficient information to debug what the job did so we can troubleshoot. If we use the json log entry with appropriate keys; hopefully we can filter logs pretty easily.

pkg/controller.v2/controller_pod.go, line 46 at r6 (raw file):

	pods = filterPodsForTFReplicaType(pods, rt)
	succeeded, failed := getPodStatus(pods)
	runningPods := filterRunningPods(pods)

Should you compute succeeded and running pods in the loop below because you want to take into account the fact that a replica could have more than 1 pod.

Why not use the for loop below to compute a map

replicaIndex -> status
and then pass that to update status?

pkg/controller.v2/controller_pod.go, line 53 at r6 (raw file):

	for index, podSlice := range podSlices {
		if len(podSlice) > 1 {
			loggerForTFJob(tfjob).Warning("We have to many pods for the worker %d", index)

More than 1 pod might not be an error. If there is 1 running pod and the rest are not running that might be fine. Might be worth noting that in the TODO as part of fixing this later.

pkg/controller.v2/controller_pod.go, line 57 at r6 (raw file):

		}
		if len(podSlice) == 0 {
			loggerForTFJob(tfjob).Infof("need to create new pod: %s-%d", rt, index)

Would be good to log this message with keys corresponding to replica type and index. This way we can easily filter log messages to see what we did/didn't do for a particular replica.

pkg/controller.v2/controller_pod.go, line 138 at r6 (raw file):

}

func getPodSlices(pods []*v1.Pod, replicas int, logger *log.Entry) [][]*v1.Pod {

Please add a comment for getPodSlices; even thought its a private function it well help future readers of the code.

Comments from Reviewable

gaocegege · 2018-04-25T01:54:17Z

pkg/controller.v2/controller_pod.go, line 138 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Please add a comment for getPodSlices; even thought its a private function it well help future readers of the code.

OK, SGTM

Comments from Reviewable

gaocegege · 2018-04-25T01:56:35Z

pkg/controller.v2/controller_pod.go, line 189 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

IME we should log sufficient information to debug what the job did so we can troubleshoot. If we use the json log entry with appropriate keys; hopefully we can filter logs pretty easily.

I added such statement for debug and removed now. I think the log in this function is enough to help us locate the errors. WDYT

Comments from Reviewable

gaocegege · 2018-04-25T01:58:39Z

pkg/controller.v2/controller_pod.go, line 190 at r2 (raw file):

Previously, ScorpioCPH (Penghao Cen) wrote…

How about use a map for podSlices, like this:
podSlices := make(map[int64][]*v1.Pod, replicas)

I considered it once but I think the index is in a range then I think slice is easier to understand.

Comments from Reviewable

gaocegege · 2018-04-25T01:59:18Z

Comments from Reviewable

gaocegege · 2018-04-25T02:00:59Z

pkg/controller.v2/controller_pod.go, line 53 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

More than 1 pod might not be an error. If there is 1 running pod and the rest are not running that might be fine. Might be worth noting that in the TODO as part of fixing this later.

I think it is an error since we expect that only one pod for one replica index is created.

Comments from Reviewable

gaocegege · 2018-04-25T02:01:16Z

pkg/controller.v2/controller_pod.go, line 46 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Should you compute succeeded and running pods in the loop below because you want to take into account the fact that a replica could have more than 1 pod.

Why not use the for loop below to compute a map

replicaIndex -> status
and then pass that to update status?

Good idea, I will update it.

Comments from Reviewable

gaocegege · 2018-04-25T02:02:01Z

pkg/controller.v2/controller_pod.go, line 57 at r6 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Would be good to log this message with keys corresponding to replica type and index. This way we can easily filter log messages to see what we did/didn't do for a particular replica.

OK, will do it.

Comments from Reviewable

gaocegege · 2018-04-25T02:02:10Z

pkg/controller.v2/controller_pod.go, line 101 at r2 (raw file):

Previously, ScorpioCPH (Penghao Cen) wrote…

What this line used for?

Removed.

Comments from Reviewable

ScorpioCPH

LGTM with some minor nits.

ScorpioCPH · 2018-04-25T03:04:53Z

pkg/controller.v2/controller_status.go

@@ -0,0 +1,66 @@
+package controller


Please add copyright here :)

Sorry for that. I will add it soon.

ScorpioCPH · 2018-04-25T03:07:35Z

pkg/controller.v2/controller_status.go

+		}
+	}
+
+	// All workers are running, set StartTime


nit: add .

Signed-off-by: Ce Gao <gaoce@caicloud.io>

gaocegege · 2018-04-25T05:25:55Z

PTAL @ScorpioCPH

yph152 · 2018-04-25T06:52:25Z

 else {
 +			// We already have one, and check the status.
 +			pod := podSlice[0]
 +			updateTFJobReplicaStatuses(tfjob, rtype, pod)
  		}		  		}
...
func updateTFJobReplicaStatuses(tfjob *tfv1alpha2.TFJob, rtype tfv1alpha2.TFReplicaType, pod *v1.Pod) {
 +	switch pod.Status.Phase {
 +	case v1.PodRunning, v1.PodPending:
 +		tfjob.Status.TFReplicaStatuses[rtype].Active++
 +	case v1.PodSucceeded:
 +		tfjob.Status.TFReplicaStatuses[rtype].Succeeded++
 +	case v1.PodFailed:
 +		tfjob.Status.TFReplicaStatuses[rtype].Failed++
 +	}
 +}

@gaocegege If the state of pod continues to change, it will always be restarted, and the state of
running --> failed --> running --> failed..., These values are going to get bigger all the time.

gaocegege · 2018-04-25T06:55:42Z

@yph152 I do not think so, because of https://github.com/kubeflow/tf-operator/pull/548/files#diff-a5453ed67196062408fa782a70f4be38R47 https://github.com/kubeflow/tf-operator/pull/548/files#diff-06337aec88b7ce18a9e200eae0058a5cR88

yph152 · 2018-04-25T07:07:39Z

thanks，I understand！

ScorpioCPH · 2018-04-25T07:49:11Z

/lgtm
/approve

k8s-ci-robot · 2018-04-25T07:49:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ScorpioCPH

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:

~~OWNERS~~ [ScorpioCPH]

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

* controller_pod: refactor Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller: Add test case Signed-off-by: Ce Gao <gaoce@caicloud.io> * WIP Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller_pod: Separate creation and status update Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller: Add continue Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller: Remove old code Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller: Remove log Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller: Update Signed-off-by: Ce Gao <gaoce@caicloud.io> * controller_status: Add copyright header Signed-off-by: Ce Gao <gaoce@caicloud.io>

k8s-ci-robot added the do-not-merge/work-in-progress label Apr 20, 2018

k8s-ci-robot requested review from ankushagarwal and lluunn April 20, 2018 11:21

k8s-ci-robot added the size/L label Apr 20, 2018

gaocegege changed the title ~~WIP: Refactor controller_pod~~ controller: Refactor controller_pod Apr 24, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress label Apr 24, 2018

k8s-ci-robot assigned jlewi and ScorpioCPH Apr 24, 2018

ankushagarwal removed their request for review April 24, 2018 03:50

ScorpioCPH reviewed Apr 24, 2018

View reviewed changes

gaocegege added 5 commits April 24, 2018 12:03

controller_pod: refactor

0582a9e

Signed-off-by: Ce Gao <gaoce@caicloud.io>

controller: Add test case

f852f07

Signed-off-by: Ce Gao <gaoce@caicloud.io>

WIP

9b5fc56

Signed-off-by: Ce Gao <gaoce@caicloud.io>

controller_pod: Separate creation and status update

75fac52

Signed-off-by: Ce Gao <gaoce@caicloud.io>

controller: Add continue

f3fbbac

Signed-off-by: Ce Gao <gaoce@caicloud.io>

controller: Remove old code

63fa3d2

Signed-off-by: Ce Gao <gaoce@caicloud.io>

gaocegege mentioned this pull request Apr 24, 2018

vendor: Use dep instead of glide and prune it #557

Merged

controller: Remove log

d6cab71

Signed-off-by: Ce Gao <gaoce@caicloud.io>

ScorpioCPH approved these changes Apr 25, 2018

View reviewed changes

gaocegege added 2 commits April 25, 2018 13:16

controller: Update

d624b47

Signed-off-by: Ce Gao <gaoce@caicloud.io>

controller_status: Add copyright header

d83e7fb

Signed-off-by: Ce Gao <gaoce@caicloud.io>

k8s-ci-robot added the lgtm label Apr 25, 2018

k8s-ci-robot added the approved label Apr 25, 2018

k8s-ci-robot merged commit 07fd2d6 into kubeflow:master Apr 25, 2018

gaocegege deleted the slice branch April 25, 2018 07:50

This was referenced Apr 27, 2018

[v1alpha2] Enhance the logic about sync #547

Closed

[v1alpha2] Potential bugs when there is one worker succeeded #538

Closed

gaocegege mentioned this pull request May 9, 2018

[v1alpha2]tfjob restartPolicy for Never #555

Closed

gaocegege mentioned this pull request May 29, 2018

service: Refactor to the slice structure #603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: Refactor controller_pod #548

controller: Refactor controller_pod #548

gaocegege commented Apr 20, 2018 •

edited by jlewi

Loading

coveralls commented Apr 20, 2018 •

edited

Loading

gaocegege commented Apr 24, 2018

ScorpioCPH left a comment

ScorpioCPH Apr 24, 2018

ScorpioCPH Apr 24, 2018

gaocegege Apr 24, 2018

ScorpioCPH Apr 24, 2018

ScorpioCPH Apr 24, 2018

ScorpioCPH Apr 24, 2018

gaocegege Apr 24, 2018

ScorpioCPH Apr 24, 2018

gaocegege commented Apr 24, 2018

jlewi commented Apr 24, 2018

jlewi commented Apr 24, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

ScorpioCPH left a comment

ScorpioCPH Apr 25, 2018

gaocegege Apr 25, 2018

ScorpioCPH Apr 25, 2018

gaocegege Apr 25, 2018

gaocegege commented Apr 25, 2018

yph152 commented Apr 25, 2018

gaocegege commented Apr 25, 2018 •

edited

Loading

yph152 commented Apr 25, 2018

ScorpioCPH commented Apr 25, 2018

k8s-ci-robot commented Apr 25, 2018

		@@ -212,9 +198,9 @@ func getPodSlices(pods []v1.Pod, replicas int, logger log.Entry) [][]*v1.Pod {
		}

controller: Refactor controller_pod #548

controller: Refactor controller_pod #548

Conversation

gaocegege commented Apr 20, 2018 • edited by jlewi Loading

coveralls commented Apr 20, 2018 • edited Loading

gaocegege commented Apr 24, 2018

ScorpioCPH 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

gaocegege commented Apr 24, 2018

jlewi commented Apr 24, 2018

jlewi commented Apr 24, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

gaocegege commented Apr 25, 2018

ScorpioCPH 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

gaocegege commented Apr 25, 2018

yph152 commented Apr 25, 2018

gaocegege commented Apr 25, 2018 • edited Loading

yph152 commented Apr 25, 2018

ScorpioCPH commented Apr 25, 2018

k8s-ci-robot commented Apr 25, 2018

gaocegege commented Apr 20, 2018 •

edited by jlewi

Loading

coveralls commented Apr 20, 2018 •

edited

Loading

gaocegege commented Apr 25, 2018 •

edited

Loading