Shared implementation of operator code #773

johnugeorge · 2018-08-08T08:38:59Z

This is a continuation of TF refactoring PR #767

Moved common initialization to the base controller
Moved common pod and service level functions to the base

This change is

coveralls · 2018-08-08T09:03:06Z

Coverage decreased (-0.1%) to 58.026% when pulling b5ced61 on johnugeorge:refactor into d2509aa on kubeflow:master.

johnugeorge · 2018-08-08T09:04:48Z

/cc @jlewi /cc @gaocegege

k8s-ci-robot · 2018-08-08T09:04:49Z

@johnugeorge: GitHub didn't allow me to request PR reviews from the following users: /cc.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jlewi /cc @gaocegege

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.

gaocegege · 2018-08-08T09:10:25Z

pkg/controller.v2/jobcontroller/jobcontroller.go

@@ -115,6 +133,75 @@ type JobController struct {
 	Recorder record.EventRecorder
 }

+func NewJobController(


The name may conflicts users since Kubernetes has a job controller

How about TrainingJobController or BaseController

Since it is package scoped, is it a problem? I initially thought of putting just "Controller" as the package name. But I felt that it will be confusing as we already have a "Controller" package for v1alpha1.

The new contributors will be confused, maybe. While it is not a big problem, just a nit.

gaocegege · 2018-08-08T09:12:30Z

pkg/controller.v2/tfcontroller/tfcontroller.go

@@ -75,7 +67,7 @@ var (
 // TFJobController is the type for TFJob Controller, which manages
 // the lifecycle of TFJobs.
 type TFJobController struct {
-	jobcontroller.JobController
+	*jobcontroller.JobController


I am not sure why using pointer here, could you please explain more about it?

No modification. Hence not necessary. I will update it.

gaocegege · 2018-08-08T09:12:53Z

Generally LGTM 😄

gaocegege · 2018-08-08T09:38:09Z

/lgtm
/hold

Waiting for @jlewi 's review

ScorpioCPH

LGTM after nits. Thanks!

ScorpioCPH · 2018-08-08T09:43:20Z

pkg/controller.v2/jobcontroller/jobcontroller.go

-	// AdoptFunc used byControlRefManager to get the latest object if UID matches
-	AdoptFunc(job metav1.Object) func() (metav1.Object, error)
+	// Returns the Controller name
+	GetControllerName() string


nit: I prefer ControllerName(), GetXxx() is Java style :)

I was thinking the same. But I left it because many interfaces in kubernetes haven't followed it.
https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/meta.go#L33

WDYT?

@ScorpioCPH Added the changes

ScorpioCPH · 2018-08-08T09:45:33Z

pkg/controller.v2/jobcontroller/pod.go

+	// orphan.
+	// for _, tfjob := range tc.getPodJobs(pod) {
+	// 	tc.enqueueTFJob(tfjob)
+	// }


Why do we need these commented out code?

I will remove them

gaocegege · 2018-08-08T10:22:53Z

/lgtm

johnugeorge · 2018-08-08T11:41:22Z

/retest

johnugeorge · 2018-08-08T16:22:28Z

/retest

johnugeorge · 2018-08-08T17:45:47Z

cluster creation is timing out
/retest

johnugeorge · 2018-08-08T18:31:15Z

@jlewi Is this a transient issue?

johnugeorge · 2018-08-09T00:23:35Z

/retest

gaocegege · 2018-08-09T03:17:53Z

Error log:

INFO:googleapiclient.discovery:URL being requested: POST https://container.googleapis.com/v1/projects/kubeflow-ci/zones/us-east1-d/clusters?alt=json
INFO:root:Creating cluster; project=kubeflow-ci, zone=us-east1-d, name=zrator-presubmit-v2-773-b5ced61-926-5207
ERROR:root:Exception occured creating cluster: <HttpError 500 when requesting https://container.googleapis.com/v1/projects/kubeflow-ci/zones/us-east
1-d/clusters?alt=json returned "Internal error encountered.">, status: 500
Traceback (most recent call last):
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v2-773-b5ced61-926-5207/src/kubeflow/testing/py/kubeflow/testing/util.py", line 168, in
create_cluster
   response = request.execute()
 File "/usr/local/lib/python2.7/dist-packages/oauth2client/_helpers.py", line 133, in positional_wrapper
   return wrapped(*args, **kwargs)
 File "/usr/local/lib/python2.7/dist-packages/googleapiclient/http.py", line 844, in execute
   raise HttpError(resp, content, uri=self.uri)
HttpError: <HttpError 500 when requesting https://container.googleapis.com/v1/projects/kubeflow-ci/zones/us-east1-d/clusters?alt=json returned "Inte
rnal error encountered.">
Traceback (most recent call last):
 File "/usr/lib/python2.7/runpy.py", line 174, in _run_module_as_main
   "__main__", fname, loader, pkg_name)
 File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
   exec code in run_globals
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v2-773-b5ced61-926-5207/src/kubeflow/tf-operator/py/deploy.py", line 360, in <module>
   main()
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v2-773-b5ced61-926-5207/src/kubeflow/tf-operator/py/deploy.py", line 356, in main
   args.func(args)
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v2-773-b5ced61-926-5207/src/kubeflow/tf-operator/py/deploy.py", line 139, in setup_clust
er
   util.create_cluster(gke, project, zone, cluster_request)
 File "/mnt/test-data-volume/kubeflow-tf-operator-presubmit-v2-773-b5ced61-926-5207/src/kubeflow/testing/py/kubeflow/testing/util.py", line 168, in
create_cluster
   response = request.execute()
 File "/usr/local/lib/python2.7/dist-packages/oauth2client/_helpers.py", line 133, in positional_wrapper
   return wrapped(*args, **kwargs)
 File "/usr/local/lib/python2.7/dist-packages/googleapiclient/http.py", line 844, in execute
   raise HttpError(resp, content, uri=self.uri)
googleapiclient.errors.HttpError: <HttpError 500 when requesting https://container.googleapis.com/v1/projects/kubeflow-ci/zones/us-east1-d/clusters?
alt=json returned "Internal error encountered.">

gaocegege · 2018-08-09T03:18:13Z

/retest

gaocegege · 2018-08-09T03:38:10Z

/cc @jlewi
The test cannot set up the cluster, and I think the problem is from the cluster. Could you please help us to locate it?

jlewi · 2018-08-09T13:46:01Z

/assign @ankushagarwal Can you review please?

johnugeorge · 2018-08-10T01:47:18Z

/retest

gaocegege · 2018-08-10T02:35:46Z

It's cool
/lgtm
/approve

gaocegege · 2018-08-10T02:36:12Z

@jlewi Do you want to review the PR? If not I think we can merge it.

jlewi · 2018-08-10T12:46:19Z

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2018-08-10T12:46:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege, jlewi

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~~ [gaocegege,jlewi]

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

johnugeorge added 4 commits August 8, 2018 13:56

Refactoring TF operator code

9596b45

Fixing go imports

146187b

Restructuring files into separate packages

c2acbf1

Shared implementation of operators

d45e5d8

k8s-ci-robot requested review from ankushagarwal and ddysher August 8, 2018 08:39

k8s-ci-robot added the size/XL label Aug 8, 2018

k8s-ci-robot requested a review from jlewi August 8, 2018 09:04

k8s-ci-robot requested a review from gaocegege August 8, 2018 09:04

gaocegege reviewed Aug 8, 2018

View reviewed changes

Pointers are not necessary

78d6a9d

k8s-ci-robot assigned gaocegege Aug 8, 2018

k8s-ci-robot added do-not-merge/hold lgtm labels Aug 8, 2018

ScorpioCPH reviewed Aug 8, 2018

View reviewed changes

Minor fixes

d5d1ad4

k8s-ci-robot removed the lgtm label Aug 8, 2018

k8s-ci-robot added the lgtm label Aug 8, 2018

Minor fixes in arguments

9178fc7

k8s-ci-robot removed the lgtm label Aug 8, 2018

ankushagarwal removed their request for review August 8, 2018 18:08

Minor fixes

b5ced61

k8s-ci-robot added lgtm approved labels Aug 10, 2018

k8s-ci-robot assigned jlewi Aug 10, 2018

k8s-ci-robot removed the do-not-merge/hold label Aug 10, 2018

k8s-ci-robot merged commit 01c76af into kubeflow:master Aug 10, 2018

jlewi mentioned this pull request Aug 13, 2018

[Request] MXNet operator support kubeflow/community#136

Closed

akgraner mentioned this pull request Jan 2, 2024

Kubeflow Steering Committee Elections - Testimonial Phase - Johnu George kubeflow/community#675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared implementation of operator code #773

Shared implementation of operator code #773

johnugeorge commented Aug 8, 2018 •

edited by jlewi

Loading

coveralls commented Aug 8, 2018 •

edited

Loading

johnugeorge commented Aug 8, 2018

k8s-ci-robot commented Aug 8, 2018

gaocegege Aug 8, 2018

gaocegege Aug 8, 2018

johnugeorge Aug 8, 2018

gaocegege Aug 8, 2018

gaocegege Aug 8, 2018

johnugeorge Aug 8, 2018

gaocegege Aug 8, 2018

gaocegege commented Aug 8, 2018

gaocegege commented Aug 8, 2018

ScorpioCPH left a comment

ScorpioCPH Aug 8, 2018

johnugeorge Aug 8, 2018

ScorpioCPH Aug 8, 2018

johnugeorge Aug 8, 2018

ScorpioCPH Aug 8, 2018

johnugeorge Aug 8, 2018

gaocegege commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 9, 2018

gaocegege commented Aug 9, 2018 •

edited

Loading

gaocegege commented Aug 9, 2018

gaocegege commented Aug 9, 2018 •

edited

Loading

jlewi commented Aug 9, 2018

johnugeorge commented Aug 10, 2018

gaocegege commented Aug 10, 2018

gaocegege commented Aug 10, 2018

jlewi commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

Shared implementation of operator code #773

Shared implementation of operator code #773

Conversation

johnugeorge commented Aug 8, 2018 • edited by jlewi Loading

coveralls commented Aug 8, 2018 • edited Loading

johnugeorge commented Aug 8, 2018

k8s-ci-robot commented Aug 8, 2018

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 Aug 8, 2018

gaocegege commented Aug 8, 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

gaocegege commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 8, 2018

johnugeorge commented Aug 9, 2018

gaocegege commented Aug 9, 2018 • edited Loading

gaocegege commented Aug 9, 2018

gaocegege commented Aug 9, 2018 • edited Loading

jlewi commented Aug 9, 2018

johnugeorge commented Aug 10, 2018

gaocegege commented Aug 10, 2018

gaocegege commented Aug 10, 2018

jlewi commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

johnugeorge commented Aug 8, 2018 •

edited by jlewi

Loading

coveralls commented Aug 8, 2018 •

edited

Loading

gaocegege commented Aug 9, 2018 •

edited

Loading

gaocegege commented Aug 9, 2018 •

edited

Loading