create launcher and worker togther #132

davidstack · 2019-07-17T01:46:40Z

for issue #131 ,this commit will create lancher and worker togther, in the init container of lancher, the init container will wait the worker untill running

This change is

k8s-ci-robot · 2019-07-17T01:46:54Z

Hi @davidstack. Thanks for your PR.

I'm waiting for a kubeflow 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

gaocegege

/assign @terrytangyuan

Did you have a try? We create launcher and workers at the same time in the PR. I am wondering what will happen if the launcher cannot find all workers.

gaocegege · 2019-07-17T01:53:44Z

cmd/kubectl-delivery/deliver_kubectl.sh

 : ${TARGET_DIR:?"Need to set TARGET_DIR, e.g. /opt/kube"}

 cp $(which kubectl) ${TARGET_DIR}
+


What's the purpose of the script here?

to check the worker is running, if the worker is in other state, the init contianer will be in loop,just waiting for the worker running

davidstack · 2019-07-17T06:47:38Z

@gaocegege in my dev env, i test ok ,it will create lancher and worker togther (waiting kube-batch schedule ).
the init continer log is
`[root@node3 ~]# docker logs -f e953b1fef837

caffe-mpi-worker-0 slots=2
1
caffe-mpi-worker-1 slots=2
2
worker name is caffe-mpi-worker-0
Unable to use a TTY - input is not a terminal or the right kind of file
hello is hello
caffe-mpi-worker-0 is running..
worker name is caffe-mpi-worker-1
Unable to use a TTY - input is not a terminal or the right kind of file
hello is hello
caffe-mpi-worker-1 is running..`

the status of mpi woker and lancher is

`[root@node2 mpi]# kubectl get pods -o wide

NAME READY STATUS RESTARTS AGE IP NODE NOMINATED NODE READINESS GATES
caffe-mpi-launcher-pssbh 0/1 Init:0/1 0 2s node2
caffe-mpi-worker-0 0/1 ContainerCreating 0 2s node2
caffe-mpi-worker-1 0/1 ContainerCreating 0 2s node3
`

gaocegege · 2019-07-17T06:52:32Z

@davidstack

Thanks, I understand the meaning of the script.

BTW, @terrytangyuan @wackxu Please take a look.

wackxu · 2019-07-23T04:18:00Z

@davidstack The CI is failed, Could you fix the CI first?

terrytangyuan · 2019-07-23T14:12:51Z

pkg/controllers/v1alpha2/mpi_job_controller.go

 				return err
 			}
-		}
+		// If the worker is ready, start the launcher.


Remove the commented code since it's no longer needed?

davidstack · 2019-07-24T14:05:52Z

@wackxu i find three test function failed.
TestLauncherRestarting
TestLauncherDoesNotExist
TestLauncherActive

but it always has this error
error syncing mpi job: jobs.batch "test-launcher" already exists

could you give me some suggestion to change this ci test? thanks

davidstack · 2019-07-29T05:53:04Z

@wackxu i change the CI ，and all tests PASSED，but it still have this error

The command "gometalinter --config=linter_config.json --vendor ./..." exited with 1.
any suggestion？ thanks

wackxu · 2019-07-29T06:17:06Z

@davidstack You can try to run gofmt -s -w yourfile to fix this.

davidstack · 2019-07-29T06:43:31Z

@wackxu thanks, everything is ok.

wackxu · 2019-07-29T11:56:53Z

pkg/controllers/v1alpha2/mpi_job_controller.go

-		// If the worker is ready, start the launcher.
-		workerReady := worker.Status.ReadyReplicas == workerReplicas
-		if workerReady && launcher == nil {
+		_, lancherErr := c.kubeClient.BatchV1().Jobs(namespace).Get(mpiJob.Name+launcherSuffix, metav1.GetOptions{})


We do not need get mpijob from kube-apiserver again, we have just get launcher above from the lister, so just judge whether the launcher is nil.

wackxu · 2019-07-29T11:58:30Z

pkg/controllers/v1alpha2/mpi_job_controller.go

 				return err
 			}
 		}
+


Remove this blank line

wackxu · 2019-07-29T12:02:13Z

pkg/controllers/v1alpha2/mpi_job_controller_test.go

 func (f *fixture) expectUpdateJobAction(d *batchv1.Job) {
 	f.kubeActions = append(f.kubeActions, core.NewUpdateAction(schema.GroupVersionResource{Resource: "jobs"}, d.Namespace, d))
 }
+func (f *fixture) expectGetJobAction(d *kubeflow.MPIJob) {


Add one blank line before the func

davidstack · 2019-08-06T02:49:32Z

@wackxu have any other problem？

gaocegege

/ok-to-test

wackxu · 2019-08-06T06:30:44Z

@davidstack Could you rebase the commit to one? then lgtm

wackxu · 2019-08-06T06:31:18Z

/assign @terrytangyuan @rongou for approval

k8s-ci-robot · 2019-08-06T06:31:19Z

@wackxu: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubeflow members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @terrytangyuan @rongou for approval

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.

davidstack · 2019-08-06T09:24:41Z

@wackxu the commit is one commit now,thanks

wackxu · 2019-08-06T09:25:23Z

/lgtm

terrytangyuan · 2019-08-13T14:58:08Z

cmd/kubectl-delivery/deliver_kubectl.sh

+done;
+
+#/etc/mpi/hostfile
+#caffe-mpi-worker-0 slots=1


Is this needed? Please remove other unneeded code/comment as well.

k8s-ci-robot · 2019-08-15T05:56:42Z

New changes are detected. LGTM label has been removed.

terrytangyuan

/approve

k8s-ci-robot · 2019-08-15T13:08:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

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~~ [terrytangyuan]

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

k8s-ci-robot requested review from cheyang and gaocegege July 17, 2019 01:46

k8s-ci-robot added the needs-ok-to-test label Jul 17, 2019

k8s-ci-robot added the size/M label Jul 17, 2019

gaocegege reviewed Jul 17, 2019

View reviewed changes

k8s-ci-robot assigned terrytangyuan Jul 17, 2019

armandmcqueen mentioned this pull request Jul 17, 2019

Launcher and workers should live on the same nodes #133

Open

terrytangyuan reviewed Jul 23, 2019

View reviewed changes

k8s-ci-robot added size/L and removed size/M labels Jul 29, 2019

wackxu reviewed Jul 29, 2019

View reviewed changes

gaocegege reviewed Aug 6, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Aug 6, 2019

k8s-ci-robot assigned rongou Aug 6, 2019

davidstack force-pushed the master branch from fbb4e50 to c100a00 Compare August 6, 2019 09:10

k8s-ci-robot assigned wackxu Aug 6, 2019

k8s-ci-robot added the lgtm label Aug 6, 2019

terrytangyuan changed the title ~~create lancher and worker togther~~ create launcher and worker togther Aug 13, 2019

terrytangyuan reviewed Aug 13, 2019

View reviewed changes

k8s-ci-robot removed the lgtm label Aug 15, 2019

create lancher and worker togher

f1fe385

davidstack force-pushed the master branch from a232afd to f1fe385 Compare August 15, 2019 06:03

terrytangyuan approved these changes Aug 15, 2019

View reviewed changes

k8s-ci-robot added the approved label Aug 15, 2019

terrytangyuan merged commit 908eb33 into kubeflow:master Aug 15, 2019

CJZheng91 mentioned this pull request Oct 3, 2019

update status to running only when both launcher and workers are ready #145

Closed

czheng94 mentioned this pull request Oct 3, 2019

update status to running only when both launcher and workers are ready #146

Merged

		: ${TARGET_DIR:?"Need to set TARGET_DIR, e.g. /opt/kube"}

		cp $(which kubectl) ${TARGET_DIR}

create launcher and worker togther #132

create launcher and worker togther #132

Uh oh!

Conversation

davidstack commented Jul 17, 2019 • edited by jlewi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 17, 2019

Uh oh!

gaocegege left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidstack commented Jul 17, 2019

Uh oh!

gaocegege commented Jul 17, 2019

Uh oh!

wackxu commented Jul 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidstack commented Jul 24, 2019

Uh oh!

davidstack commented Jul 29, 2019

Uh oh!

wackxu commented Jul 29, 2019

Uh oh!

davidstack commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidstack commented Aug 6, 2019

Uh oh!

gaocegege left a comment

Choose a reason for hiding this comment

Uh oh!

wackxu commented Aug 6, 2019

Uh oh!

wackxu commented Aug 6, 2019

Uh oh!

k8s-ci-robot commented Aug 6, 2019

Uh oh!

davidstack commented Aug 6, 2019

Uh oh!

wackxu commented Aug 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 15, 2019

Uh oh!

terrytangyuan left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Aug 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

davidstack commented Jul 17, 2019 •

edited by jlewi

Loading

gaocegege left a comment •

edited

Loading