Import v1alpha2 logic code #463

ScorpioCPH · 2018-03-15T02:43:09Z

Hi, this PR implement the main logic of this design proposal kubeflow/community#30

Redefine TFJobController struct.
Use event-driven and reconcile to create new pods and services.
- Copy some utils code from k8s.io/kubernetes/pkg/controller.
- We should not import the huge package k8s.io/kubernetes/pkg/controller
Update examples for test
- Use busybox instead of real TensorFlow model for test now.

TODOs:

Add error handling logic.
Update status gracefully.
Add UT.

@jlewi @gaocegege @lluunn @jimexist @DjangoPeng @ddysher PTAL.

Note: Please do not use squash merge as this is a huge PR, let's keep the commits history, thanks!

This change is

gaocegege

Generally LGTM with some issues about name.

gaocegege · 2018-03-15T02:47:08Z

pkg/util/k8sutil/k8sutil.go

-	if len(os.Getenv(RecommendedConfigPathEnvVar)) > 0 {
-		// use the current context in kubeconfig
-		// This is very useful for running locally.
-		return clientcmd.BuildConfigFromFlags("", os.Getenv(RecommendedConfigPathEnvVar))


I think we could keep the logic for dev locally.

Add this in server.go, PTAL.

gaocegege · 2018-03-15T02:48:15Z

pkg/controller/controller.go

 )

 const (
-	controllerName = "kubeflow"
-)
+	controllerName = "tfjob-controller"


I think we could name it tf-operator?

gaocegege · 2018-03-15T02:52:57Z

pkg/controller/controller.go

-	config tfv1alpha1.ControllerConfig, tfJobInformerFactory informers.SharedInformerFactory) (*Controller, error) {
-	tfJobInformer := tfJobInformerFactory.Kubeflow().V1alpha1().TFJobs()
+// NewTFJobController returns a new TFJob controller.
+func NewTFJobController(


Could you give me more info about why the name of the function is changed to NewTFJobController? I think New is fine in this case.

Keep the style as kubernetes controllers.

gaocegege · 2018-03-15T02:54:12Z

pkg/controller/controller.go

+	ReconcilerSyncLoopPeriod: metav1.Duration{Duration: 15 * time.Second},
+}
+
+type TFJobController struct {


I think Controller or Operator is enough, since this repo is only for TFJob, WDYT

Keep the style as kubernetes controllers.

gaocegege · 2018-03-15T02:54:39Z

pkg/controller/controller.go


 	// Start the informer factories to begin populating the informer caches
 	log.Info("Starting TFJob controller")

 	// Wait for the caches to be synced before starting workers
 	log.Info("Waiting for informer caches to sync")
-	if ok := cache.WaitForCacheSync(stopCh, c.TFJobSynced); !ok {
+	if ok := cache.WaitForCacheSync(stopCh, tc.tfJobListerSynced); !ok {


Should we wait pod and service to be synced?

Yes, thanks.

gaocegege · 2018-03-15T02:56:47Z

pkg/controller/controller.go

+	if controllerRef.Kind != controllerKind.Kind {
+		return nil
+	}
+	job, err := tc.tfJobLister.TFJobs(namespace).Get(controllerRef.Name)


s/job/tfjob

gaocegege · 2018-03-15T03:00:55Z

pkg/controller/controller_ref_manager.go

+	return true, nil
+}
+
+type PodControllerRefManager struct {


I am wondering if we could use the ref manager in "k8s.io/kubernetes/pkg/controller".

Add Note in the beginning of this file:

// Note(CPH): this file is copied form k8s.io/kubernetes/pkg/controller // We should not import the huge package k8s.io/kubernetes/pkg

gaocegege · 2018-03-15T03:05:44Z

pkg/controller/controller_service.go

@@ -0,0 +1,207 @@
+// Copyright 2018 The Kubeflow Authors


I think the file name has redundancy, could we name it pkg/controller/service.go

Actually, this file is split from pkg/controller/controller.go.

gaocegege · 2018-03-15T03:09:30Z

pkg/controller/controller_utils.go

+// Only abstracted out for testing.
+// Warning: if using KeyFunc it is not safe to use a single ControllerExpectationsInterface with different
+// types of controllers, because the keys might conflict across types.
+type ControllerExpectationsInterface interface {


Why should we write our own ControllerExpectationsInterface? I think we could reuse the data structure in k8s code.

This file is copied form k8s.io/kubernetes/pkg/controller.
k8s.io/kubernetes/pkg is used internal and we should not import the huge package k8s.io/kubernetes/pkg.

gaocegege · 2018-03-15T03:10:18Z

pkg/controller/controller_utils.go

+// RSControlInterface is an interface that knows how to add or delete
+// ReplicaSets, as well as increment or decrement them. It is used
+// by the deployment controller to ease testing of actions that it takes.
+type RSControlInterface interface {


Do we need this interface? I think we have no plan for ReplicaSet.

Yes, will remove this.

k8s-ci-robot · 2018-03-15T03:14:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: gaocegege

Assign the PR to them by writing /assign @gaocegege in a comment when ready.

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

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

gaocegege · 2018-03-15T06:08:58Z

Generally LGTM but please fix the test in travis. Then I will merge it manually.

gaocegege · 2018-03-16T01:51:05Z

I will merge it first since it blocks my work on test #455

k8s-ci-robot requested review from ankushagarwal and zjj2wry March 15, 2018 02:43

k8s-ci-robot added the size/XXL label Mar 15, 2018

ScorpioCPH assigned gaocegege Mar 15, 2018

gaocegege reviewed Mar 15, 2018

View reviewed changes

ScorpioCPH force-pushed the v1alpha2 branch from 9847ab9 to 60bcad4 Compare March 15, 2018 03:14

ScorpioCPH added 2 commits March 15, 2018 11:50

Add options kubeconfig/master/threadiness and remove deprecated options

cdbf139

Update copyright with new boilerplate

5c1fb57

ScorpioCPH force-pushed the v1alpha2 branch 2 times, most recently from b03b2b8 to fee07ae Compare March 15, 2018 06:03

ScorpioCPH force-pushed the v1alpha2 branch from fee07ae to 43f0ec3 Compare March 15, 2018 08:24

Cleanup v1alpha1 code

01a36a9

ScorpioCPH force-pushed the v1alpha2 branch from 94da6b9 to 5faef78 Compare March 15, 2018 08:32

ScorpioCPH added 2 commits March 15, 2018 17:07

Import v1alpha2 logic code

073ed6d

Update examples for test

f467354

ScorpioCPH force-pushed the v1alpha2 branch 2 times, most recently from 5a23d8e to 9b8c398 Compare March 15, 2018 09:36

Fix travis-ci error

9b8cfea

ScorpioCPH force-pushed the v1alpha2 branch from 9b8c398 to 9b8cfea Compare March 15, 2018 10:10

ankushagarwal removed their request for review March 16, 2018 00:29

gaocegege merged commit 1463dba into kubeflow:v1alpha2 Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import v1alpha2 logic code #463

Import v1alpha2 logic code #463

ScorpioCPH commented Mar 15, 2018 •

edited by jlewi

Loading

gaocegege left a comment

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

gaocegege Mar 15, 2018

ScorpioCPH Mar 15, 2018

k8s-ci-robot commented Mar 15, 2018

gaocegege commented Mar 15, 2018

gaocegege commented Mar 16, 2018

Import v1alpha2 logic code #463

Import v1alpha2 logic code #463

Conversation

ScorpioCPH commented Mar 15, 2018 • edited by jlewi Loading

gaocegege 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

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

k8s-ci-robot commented Mar 15, 2018

gaocegege commented Mar 15, 2018

gaocegege commented Mar 16, 2018

ScorpioCPH commented Mar 15, 2018 •

edited by jlewi

Loading