pkg: Add recorder support #312

gaocegege · 2018-01-15T09:01:40Z

This PR focus on recorder support in Kubernetes. Now we could push events to Kubernetes about the TFJob:

Events:
  Type    Reason            Age   From      Message
  ----    ------            ----  ----      -------
  Normal  SuccessfulCreate  9s    kubeflow  Created service: example-job-master-xpnw-0
  Normal  SuccessfulCreate  9s    kubeflow  Created job: example-job-master-xpnw-0
  Normal  SuccessfulCreate  9s    kubeflow  Created service: example-job-worker-xpnw-0
  Normal  SuccessfulCreate  9s    kubeflow  Created job: example-job-worker-xpnw-0

Signed-off-by: Ce Gao [email protected]

This change is

k8s-ci-robot · 2018-01-15T09:01:54Z

Hi @gaocegege. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

coveralls · 2018-01-15T09:56:08Z

Coverage increased (+0.07%) to 31.68% when pulling 1968e71 on gaocegege:recorder into 98a34a1 on tensorflow:master.

ScorpioCPH

Generally LGTM with some nits.

ScorpioCPH · 2018-01-15T09:36:25Z

pkg/trainer/replicas.go

 			return err
+		} else {


nit: we have returned in the previous if statement, so else condition can be removed here.

ScorpioCPH · 2018-01-15T09:36:42Z

pkg/trainer/replicas.go

 			}
+		} else {


There is a subbranch https://github.com/tensorflow/k8s/pull/312/files/47249c0893c5be0c9e86f3f77ebbb0ea66c1726a#diff-d2bc8c1807fa25d2b911d2d781f48a07L178.

And if we remove else, there will be some idempotent events

I mean the else at line 195, not line 191.

Yeah, I know. But if we remove the else, the branch that the service already exists will publish events, too.

ScorpioCPH · 2018-01-15T09:38:48Z

pkg/trainer/replicas.go

 			}
+		} else {


ScorpioCPH · 2018-01-15T09:43:26Z

pkg/trainer/replicas.go

 		if err != nil {
-			log.Errorf("Error creating PS ConfigMap: %v, %v", cm.ObjectMeta.Name, err)
+			log.Errorf("Error creating PS ConfigMap: %v, %v", createdCM.ObjectMeta.Name, err)
+			s.recorder.Eventf(s.Job.job, v1.EventTypeWarning, FailedCreateReason, "Error creating: %v", err)


Maybe we can add more info here: Error creating configmaps: xxx

ScorpioCPH · 2018-01-15T09:44:10Z

pkg/trainer/replicas.go


 		// If the job already exists do nothing.
 		if err != nil {
 			if k8s_errors.IsAlreadyExists(err) {
 				log.Infof("Service %v already exists.", s.jobName(index))
 			} else {
-				return k8sErrors.NewAggregate([]error{fmt.Errorf("Creating service %v returned error.", service.ObjectMeta.Name), err})
+				s.recorder.Eventf(s.Job.job, v1.EventTypeWarning, FailedCreateReason, "Error creating: %v", err)


Error creating service xxx

ScorpioCPH · 2018-01-15T09:44:40Z

pkg/trainer/replicas.go


 		// If the job already exists do nothing.
 		if err != nil {
 			if k8s_errors.IsAlreadyExists(err) {
 				log.Infof("%v already exists.", s.jobName(index))

 			} else {
-				return k8sErrors.NewAggregate([]error{fmt.Errorf("Creating Job %v returned error.", newJ.ObjectMeta.Name), err})
+				s.recorder.Eventf(s.Job.job, v1.EventTypeWarning, FailedCreateReason, "Error creating: %v", err)


Error creating job: xxx

coveralls · 2018-01-15T10:19:08Z

Coverage increased (+0.07%) to 31.68% when pulling 8431e13 on gaocegege:recorder into 98a34a1 on tensorflow:master.

jlewi · 2018-01-15T18:09:30Z

Can you update the PR description to explain what a recorder does? I suspect it records events and publishes them?

jlewi · 2018-01-15T18:12:47Z

/ok-to-test

gaocegege · 2018-01-16T03:55:02Z

@jlewi

The recorder records events for the TFJob instance. For example, if we create a TFJob, we need to create some services and jobs for the TFJob. Then we record the creations for these via the recorder.

jlewi · 2018-01-16T13:01:27Z

@gaocegege Can you open up an issue to add E2E tests to verify the events are published?

Please sync but otherwise LGTM.

gaocegege · 2018-01-16T16:12:53Z

@jlewi OK, and I can take the issue.

ScorpioCPH · 2018-01-17T02:08:41Z

@gaocegege Need rebase :)

googlebot · 2018-01-19T08:30:09Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Signed-off-by: Ce Gao <[email protected]>

googlebot · 2018-01-19T08:32:28Z

CLAs look good, thanks!

coveralls · 2018-01-19T08:38:44Z

Coverage increased (+0.2%) to 31.611% when pulling 2ec6bc7 on gaocegege:recorder into 04425d9 on tensorflow:master.

coveralls · 2018-01-19T08:41:39Z

Coverage increased (+0.2%) to 31.611% when pulling 2ec6bc7 on gaocegege:recorder into 04425d9 on tensorflow:master.

coveralls · 2018-01-19T08:46:46Z

Coverage increased (+0.2%) to 31.611% when pulling 2ec6bc7 on gaocegege:recorder into 04425d9 on tensorflow:master.

gaocegege · 2018-01-19T08:58:49Z

PTAL

jlewi · 2018-01-20T02:07:41Z

@ScorpioCPH Have your comments been addressed?

ScorpioCPH · 2018-01-21T14:22:25Z

@jlewi @gaocegege Mostly, only some nits, LGTM.

k8s-ci-robot added the needs-ok-to-test label Jan 15, 2018

gaocegege mentioned this pull request Jan 15, 2018

[discussion] Differences between tensorflow/k8s and caicloud/kubeflow-controller #283

Closed

6 tasks

ScorpioCPH approved these changes Jan 15, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test label Jan 15, 2018

gaocegege mentioned this pull request Jan 16, 2018

[enhancement] Add e2e test cases for recorder #317

Closed

*: Add recorder

2ec6bc7

Signed-off-by: Ce Gao <[email protected]>

jlewi approved these changes Jan 20, 2018

View reviewed changes

jlewi merged commit ca638ed into kubeflow:master Jan 22, 2018

gaocegege deleted the recorder branch April 10, 2018 03:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg: Add recorder support #312

pkg: Add recorder support #312

gaocegege commented Jan 15, 2018 •

edited by jlewi

Loading

k8s-ci-robot commented Jan 15, 2018

coveralls commented Jan 15, 2018 •

edited

Loading

ScorpioCPH left a comment

ScorpioCPH Jan 15, 2018

ScorpioCPH Jan 15, 2018

gaocegege Jan 15, 2018

ScorpioCPH Jan 15, 2018

gaocegege Jan 19, 2018

ScorpioCPH Jan 15, 2018

ScorpioCPH Jan 15, 2018

gaocegege Jan 15, 2018

ScorpioCPH Jan 15, 2018

ScorpioCPH Jan 15, 2018

coveralls commented Jan 15, 2018 •

edited

Loading

jlewi commented Jan 15, 2018

jlewi commented Jan 15, 2018

gaocegege commented Jan 16, 2018

jlewi commented Jan 16, 2018

gaocegege commented Jan 16, 2018

ScorpioCPH commented Jan 17, 2018

googlebot commented Jan 19, 2018

googlebot commented Jan 19, 2018

coveralls commented Jan 19, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

gaocegege commented Jan 19, 2018

jlewi commented Jan 20, 2018

ScorpioCPH commented Jan 21, 2018

pkg: Add recorder support #312

pkg: Add recorder support #312

Conversation

gaocegege commented Jan 15, 2018 • edited by jlewi Loading

k8s-ci-robot commented Jan 15, 2018

coveralls commented Jan 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 15, 2018 • edited Loading

jlewi commented Jan 15, 2018

jlewi commented Jan 15, 2018

gaocegege commented Jan 16, 2018

jlewi commented Jan 16, 2018

gaocegege commented Jan 16, 2018

ScorpioCPH commented Jan 17, 2018

googlebot commented Jan 19, 2018

googlebot commented Jan 19, 2018

coveralls commented Jan 19, 2018 • edited Loading

coveralls commented Jan 19, 2018 • edited Loading

coveralls commented Jan 19, 2018 • edited Loading

gaocegege commented Jan 19, 2018

jlewi commented Jan 20, 2018

ScorpioCPH commented Jan 21, 2018

gaocegege commented Jan 15, 2018 •

edited by jlewi

Loading

coveralls commented Jan 15, 2018 •

edited

Loading

coveralls commented Jan 15, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading