Add a field SchedulerName to TFJob for specifying a scheduler #408

mitake · 2018-02-26T05:25:42Z

This commit adds a new field SchedulerName to the definition of TFJob.
The purpose of the field is specifying the scheduler name of the pods
created by tf-operator and let the scheduler (which wouldn't be the
default scheduler) handle them. It would be convenient for letting
kube-batchd (a component of kube-arbitrator) handle the pods.

/cc @jlewi this is a newer version of #398

This change is

coveralls · 2018-02-26T05:38:44Z

Coverage increased (+0.2%) to 28.606% when pulling 445f01b on mitake:sched-name-tfjob into 997c583 on kubeflow:master.

gaocegege

Do we need to define a default scheduler for TFJob?

mitake · 2018-02-26T06:01:03Z

@gaocegege probably we don't need to define. If the field is empty, apiserver will supply the name of the default scheduler (default-scheduler) to a pod.

gaocegege · 2018-02-26T06:12:36Z

/ok-to-test

jlewi · 2018-02-26T20:53:33Z

Why is scheduler name a property of the replica vs. a property of the job?

I was expecting this PR to enable testing of the use of kube-arbitrartor. I was expecting that to be a job level setting as opposed to a replica level setting.

gaocegege · 2018-02-27T01:35:17Z

I think all the TFJob's replicas will be scheduled by one scheduler, which may be kube-arbitrator. Then Job level SGTM.

mitake · 2018-02-27T06:43:12Z

I was expecting this PR to enable testing of the use of kube-arbitrartor. I was expecting that to be a job level setting as opposed to a replica level setting.

This PR provides a job level setting. The job level setting will be propagated to the replica level setting during the creation process. If I'm misunderstanding something, sorry for that and please point out.

jlewi · 2018-02-27T12:19:42Z

Thanks. Looks good; must have misread it before.

Should we add a unittest to verify pod scheduler is set properly?

mitake · 2018-02-27T12:22:06Z

Yes, the unittest would be valuable. I'll add it tomorrow.

gaocegege · 2018-02-28T03:28:59Z

pkg/trainer/tensorboard.go

@@ -178,6 +178,8 @@ func (s *TBReplicaSet) getDeploymentSpecTemplate(image string) v1.PodTemplateSpe

 	ps.Volumes = append(ps.Volumes, s.Spec.Volumes...)

+	ps.SchedulerName = s.Job.SchedulerName()


There is no TensorBoard now so we could remove the changes here.

OK, I'll remove it in the next update. Thanks.

mitake · 2018-02-28T06:26:15Z

@jlewi @gaocegege updated for

unit test
removing the change related to tensorboard

Could you take a look?

jlewi

/approve

gaocegege · 2018-02-28T14:12:44Z

/lgtm

wackxu · 2018-03-01T03:35:02Z

You should run hack/update-codegen.sh to generate code and we should add hack/verify-codegen.sh into our test @gaocegege @jlewi

gaocegege · 2018-03-01T03:39:52Z

@wackxu

Thanks and I agree that. I will file an issue for it.

mitake · 2018-03-01T07:45:48Z

Probably the CI is failing because of invalid format of python files. I created another PR for the issue: #429

jlewi · 2018-03-01T13:49:42Z

@ScorpioCPH @mitake This change likely conflicts with #344. Which PR should be submitted first

/hold

ScorpioCPH

@mitake Thanks for your PR, LGTM for adding this field.
But i think we don't need a special Getter for SchedulerName.

ScorpioCPH · 2018-03-01T14:52:59Z

pkg/trainer/training.go

@@ -405,3 +405,7 @@ func (j *TrainingJob) name() string {
 func (j *TrainingJob) fullname() string {
 	return j.job.ObjectMeta.GetNamespace() + ":" + j.job.ObjectMeta.GetName()
 }
+
+func (j *TrainingJob) SchedulerName() string {
+	return j.job.Spec.SchedulerName


Why do we need a special Getter for SchedulerName?
We can just get it directly form j.job.Spec.SchedulerName.

Because TrainingJob.job is a private member so TrainingJob should provide the accessor method.

Yes, the current structure is a little complex:

We have TFJob for API object Spec/Status

Then create TrainingJob and TFReplicaSet which keep many cache of TFJob

And PodSpec already have a field SchedulerName, can we reuse it?

PodSpec.SchedulerName isn't suitable for the purpose. See the discussion start with this message #398 (comment)

gaocegege · 2018-03-06T03:12:07Z

@mitake Thanks for your contribution! Could you rebase the master and I think we could merge it ASAP.

This commit adds a new field SchedulerName to the definition of TFJob. The purpose of the field is specifying the scheduler name of the pods created by tf-operator and let the scheduler (which wouldn't be the default scheduler) handle them. It would be convenient for letting kube-batchd (a component of kube-arbitrator) handle the pods.

mitake · 2018-03-07T01:33:36Z

@gaocegege rebased on the latest master, PTAL

gaocegege · 2018-03-07T03:51:24Z

/lgtm

k8s-ci-robot · 2018-03-07T03:51:30Z

[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

gaocegege · 2018-03-07T03:51:43Z

@ScorpioCPH Do you want to have another review?

gaocegege · 2018-03-07T06:26:43Z

I will merge it soon since there is no more review.

k8s-ci-robot added needs-ok-to-test size/S labels Feb 26, 2018

gaocegege reviewed Feb 26, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test label Feb 26, 2018

gaocegege reviewed Feb 28, 2018

View reviewed changes

gaocegege mentioned this pull request Feb 28, 2018

Kubeflow 周报 gaocegege/Blog#7

Closed

mitake force-pushed the sched-name-tfjob branch from 6b621e2 to 0b807f3 Compare February 28, 2018 06:25

jlewi approved these changes Feb 28, 2018

View reviewed changes

k8s-ci-robot added the approved label Feb 28, 2018

k8s-ci-robot assigned gaocegege Feb 28, 2018

k8s-ci-robot added the lgtm label Feb 28, 2018

gaocegege mentioned this pull request Mar 1, 2018

clientset: Fix code which is changed manually #428

Merged

k8s-ci-robot added the do-not-merge/hold label Mar 1, 2018

ScorpioCPH suggested changes Mar 1, 2018

View reviewed changes

mitake force-pushed the sched-name-tfjob branch from 0b807f3 to 445f01b Compare March 7, 2018 01:33

k8s-ci-robot removed the lgtm label Mar 7, 2018

k8s-ci-robot added the lgtm label Mar 7, 2018

gaocegege merged commit a4b8031 into kubeflow:master Mar 7, 2018

mitake deleted the sched-name-tfjob branch March 7, 2018 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a field SchedulerName to TFJob for specifying a scheduler #408

Add a field SchedulerName to TFJob for specifying a scheduler #408

mitake commented Feb 26, 2018 •

edited by jlewi

Loading

coveralls commented Feb 26, 2018 •

edited

Loading

gaocegege left a comment

mitake commented Feb 26, 2018

gaocegege commented Feb 26, 2018

jlewi commented Feb 26, 2018

gaocegege commented Feb 27, 2018

mitake commented Feb 27, 2018

jlewi commented Feb 27, 2018

mitake commented Feb 27, 2018

gaocegege Feb 28, 2018

mitake Feb 28, 2018

mitake commented Feb 28, 2018

jlewi left a comment

gaocegege commented Feb 28, 2018

wackxu commented Mar 1, 2018

gaocegege commented Mar 1, 2018

mitake commented Mar 1, 2018

jlewi commented Mar 1, 2018

ScorpioCPH left a comment

ScorpioCPH Mar 1, 2018

mitake Mar 2, 2018

ScorpioCPH Mar 2, 2018

mitake Mar 2, 2018

gaocegege commented Mar 6, 2018

mitake commented Mar 7, 2018

gaocegege commented Mar 7, 2018

k8s-ci-robot commented Mar 7, 2018

gaocegege commented Mar 7, 2018

gaocegege commented Mar 7, 2018

		@@ -178,6 +178,8 @@ func (s *TBReplicaSet) getDeploymentSpecTemplate(image string) v1.PodTemplateSpe

		ps.Volumes = append(ps.Volumes, s.Spec.Volumes...)

		ps.SchedulerName = s.Job.SchedulerName()

Add a field SchedulerName to TFJob for specifying a scheduler #408

Add a field SchedulerName to TFJob for specifying a scheduler #408

Conversation

mitake commented Feb 26, 2018 • edited by jlewi Loading

coveralls commented Feb 26, 2018 • edited Loading

gaocegege left a comment

Choose a reason for hiding this comment

mitake commented Feb 26, 2018

gaocegege commented Feb 26, 2018

jlewi commented Feb 26, 2018

gaocegege commented Feb 27, 2018

mitake commented Feb 27, 2018

jlewi commented Feb 27, 2018

mitake commented Feb 27, 2018

gaocegege Feb 28, 2018

Choose a reason for hiding this comment

mitake Feb 28, 2018

Choose a reason for hiding this comment

mitake commented Feb 28, 2018

jlewi left a comment

Choose a reason for hiding this comment

gaocegege commented Feb 28, 2018

wackxu commented Mar 1, 2018

gaocegege commented Mar 1, 2018

mitake commented Mar 1, 2018

jlewi commented Mar 1, 2018

ScorpioCPH left a comment

Choose a reason for hiding this comment

ScorpioCPH Mar 1, 2018

Choose a reason for hiding this comment

mitake Mar 2, 2018

Choose a reason for hiding this comment

ScorpioCPH Mar 2, 2018

Choose a reason for hiding this comment

mitake Mar 2, 2018

Choose a reason for hiding this comment

gaocegege commented Mar 6, 2018

mitake commented Mar 7, 2018

gaocegege commented Mar 7, 2018

k8s-ci-robot commented Mar 7, 2018

gaocegege commented Mar 7, 2018

gaocegege commented Mar 7, 2018

mitake commented Feb 26, 2018 •

edited by jlewi

Loading

coveralls commented Feb 26, 2018 •

edited

Loading