Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert launcher job and statefulset worker to pod #203

Merged
merged 5 commits into from May 5, 2020
Merged

Convert launcher job and statefulset worker to pod #203

merged 5 commits into from May 5, 2020

Conversation

carmark
Copy link
Member

@carmark carmark commented Mar 19, 2020 •
edited by jlewi

fix #201

need to make sure whether we need:

  • BackoffLimit
  • ActiveDeadlineSeconds

This change is Reviewable

@carmark carmark changed the title [wip]convert launcher from job to pod convert launcher job and statefulset worker to pod Apr 3, 2020
@terrytangyuan terrytangyuan changed the title convert launcher job and statefulset worker to pod [WIP] Convert launcher job and statefulset worker to pod Apr 17, 2020
@carmark carmark changed the title [WIP] Convert launcher job and statefulset worker to pod Convert launcher job and statefulset worker to pod Apr 22, 2020
@carmark
Copy link
Member Author

carmark commented Apr 22, 2020

@terrytangyuan Could you please take a look? I move the previous patch to v1.

terrytangyuan
terrytangyuan reviewed Apr 22, 2020
View reviewed changes
deploy/v1/mpi-operator.yaml Outdated Show resolved Hide resolved
pkg/controllers/v1/mpi_job_controller_status.go Outdated Show resolved Hide resolved
pkg/apis/kubeflow/v1/constants.go Outdated Show resolved Hide resolved
cmd/mpi-operator.v1/app/server.go Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@terrytangyuan
Copy link
Member

terrytangyuan commented Apr 22, 2020 •
edited

PTAL
/cc @gaocegege @rongou

@terrytangyuan
Copy link
Member

@carmark There's already a yaml file in https://github.com/kubeflow/mpi-operator/blob/2b68b44e90fb90f6433329b5a57deb59d67bfcff/deploy/mpi-operator-v1.yaml

@gaocegege
Copy link
Member

/lgtm

@terrytangyuan
Copy link
Member

Also CI build failed: https://travis-ci.org/github/kubeflow/mpi-operator/builds/678854834

@carmark
Copy link
Member Author

carmark commented Apr 29, 2020

Thanks @terrytangyuan, I will update the unit test.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 29, 2020
@terrytangyuan
Copy link
Member

Could you also remove the duplicated manifest?

@carmark
Copy link
Member Author

carmark commented Apr 29, 2020

sure, will do it.

@carmark carmark force-pushed the job2pod branch 7 times, most recently from dc16460 to 7353055 Compare May 5, 2020 11:52
terrytangyuan
terrytangyuan approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot
Copy link

[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:

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

@terrytangyuan
Copy link
Member

/hold

@terrytangyuan
Copy link
Member

@carmark Build is still failing. Could you take another look? https://travis-ci.org/github/kubeflow/mpi-operator/pull_requests

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 5, 2020
@carmark
Copy link
Member Author

carmark commented May 5, 2020

@terrytangyuan thanks, could you please review it again? I fixed the conflict and unit test issues.

@terrytangyuan
Copy link
Member

/lgtm
/unhold

@k8s-ci-robot k8s-ci-robot merged commit 445cb48 into kubeflow:master May 5, 2020
@terrytangyuan terrytangyuan mentioned this pull request May 12, 2020
Closed
@terrytangyuan terrytangyuan mentioned this pull request Jul 6, 2020
Closed
@alculquicondor alculquicondor mentioned this pull request Feb 24, 2021
Closed
alculquicondor
alculquicondor reviewed Apr 27, 2021
View reviewed changes
pkg/controllers/v1/mpi_job_controller.go
switch worker[i].Status.Phase {
case corev1.PodFailed:
mpiJob.Status.ReplicaStatuses[common.ReplicaType(kubeflow.MPIReplicaTypeWorker)].Failed += 1
if worker[i].Status.Reason == "Evicted" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if it failed for a different reason?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@everpeace everpeace Awaiting requested review from everpeace

@gaocegege gaocegege Awaiting requested review from gaocegege

@rongou rongou Awaiting requested review from rongou

Assignees

@terrytangyuan terrytangyuan

@gaocegege gaocegege

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace Launcher from v1.Job to v1.Pod
5 participants
@carmark @terrytangyuan @gaocegege @k8s-ci-robot @alculquicondor