Run workers first and wait for them #484

xhejtman · 2022-12-13T12:01:29Z

Signed-off-by: Lukas Hejtmanek [email protected]

this is patch so that launcher waits for workers. Code works. Missing part is how to let it be configurable. Perhaps add an option into CRD at the same level as sshAuthMountPath? E.g., waitForWorkers: true|false?

alculquicondor · 2022-12-13T14:32:30Z

yes, please add a configuration option.

Maybe the boolean is fine. Another proposal could be something along these lines:

launcherStartup: OnCreation|AfterWorkersReady

But the boolean might be more readable.

v2/pkg/controller/mpi_job_controller.go

alculquicondor · 2022-12-19T15:28:22Z

Fix lint errors.
Other than that @ahg-g said he could have a look

v2/pkg/controller/mpi_job_controller.go

xhejtman · 2022-12-19T15:31:45Z

Any hit how to fix them? It seems to be in file that I didn't touch at all..

alculquicondor · 2022-12-19T16:28:22Z

oh, it just looks like the linter is broken

@terrytangyuan do you know how to upgrade it?

terrytangyuan · 2022-12-20T00:46:49Z

oh, it just looks like the linter is broken

@terrytangyuan do you know how to upgrade it?

Should be able to upgrade here: https://github.com/kubeflow/mpi-operator/blob/master/.github/workflows/golangci-lint.yml#L19

alculquicondor · 2022-12-20T14:16:54Z

@xhejtman I upgraded the linter #485
Can you rebase?

ahg-g

Can you add tests please?

ahg-g · 2022-12-22T01:06:15Z

v2/pkg/controller/mpi_job_controller.go

+	var (
+		launcherPodsCnt int
+	)


launcherPodsCnt := 0

ahg-g · 2022-12-22T01:09:55Z

v2/pkg/controller/mpi_job_controller.go

+func (c *MPIJobController) countReadyWorkerPods(workers []*corev1.Pod) (int) {
+	ready := 0
+	for _, pod := range workers {
+		for _, c := range pod.Status.Conditions {


you can replace this loop with IsStatusConditionTrue(pod.Status.Conditions, corev1.PodReady) from pkg "k8s.io/apimachinery/pkg/api/meta"

it does not seem to:

cannot convert pod.Status.Conditions (type []"k8s.io/api/core/v1".PodCondition) to type []"k8s.io/apimachinery/pkg/apis/meta/v1".Condition

ahg-g · 2022-12-22T01:25:57Z

v2/pkg/controller/mpi_job_controller.go

+					return fmt.Errorf("creating launcher Pod: %w", err)
+				}
+			} else {
+				klog.Infof("Waiting for workers to start...")


use klog.V(4) and indicate the job name/namespace pls

ahg-g · 2022-12-22T01:34:56Z

v2/pkg/apis/kubeflow/v2beta1/types.go

@@ -55,6 +55,9 @@ type MPIJobSpec struct {
 	// +kubebuilder:default:="/root/.ssh"
 	SSHAuthMountPath string `json:"sshAuthMountPath,omitempty"`

+	// Spawn launcher after all workers are in Ready state if true


The comment needs to start with the parameter name WaitForWorkers; something like:

Suggested change

// Spawn launcher after all workers are in Ready state if true

// WaitForWorkers if true, the launcher is created only after all workers are in Ready state

ahg-g · 2022-12-22T01:36:25Z

v2/pkg/controller/mpi_job_controller.go

+					return fmt.Errorf("creating launcher Pod: %w", err)
+				}
+			} else {
+				klog.Infof("Waiting for workers to start...")


what event will trigger reconciling this job to create the launcher later?

I think this is triggered by pod events (running/ready), but not quite sure, however, tested and it is triggered.

xhejtman · 2023-01-11T18:26:41Z

Can we proceed here?

tenzen-y · 2023-02-22T15:28:18Z

Can we proceed here?

@xhejtman, Can you rebase this PR? We moved all codes for v2 to the top of the repository.

alculquicondor · 2023-04-04T20:18:39Z

I guess this will have to wait for the next release

tenzen-y · 2023-04-04T20:21:43Z

I guess this will have to wait for the next release

I agree. if @xhejtman has no progress, I will take over this PR.

xhejtman · 2023-04-04T20:27:33Z

Sorry for delay. I can rebase it, if you you already know what to change, please let me know, there were some proposed changes but I think there were not possible or I am not skilled enough to do them in go ;)

alculquicondor · 2023-04-04T20:32:07Z

The thing is that we are trying to cut the release today and I rather not delay it further. If you still have time to follow up on the rebase, you can certainly do so. But we wouldn't merge until a couple of days from now.

xhejtman · 2023-04-04T21:33:00Z

I did rebase but some tests are failing. Can you advise how to fix it?

Signed-off-by: Lukas Hejtmanek <[email protected]>

xhejtman · 2023-04-04T22:06:57Z

I cleaned the mess of merges. However, tests are really failing.

Signed-off-by: Lukas Hejtmanek <[email protected]>

alculquicondor · 2023-06-22T21:00:29Z

rebase instead of merge

Signed-off-by: Lukas Hejtmanek <[email protected]>

tenzen-y

@xhejtman Thanks for the updates! I left a comment for a nit.

pkg/apis/kubeflow/v2beta1/types.go

Signed-off-by: Lukas Hejtmanek <[email protected]>

tenzen-y

@xhejtman Thanks!
/lgtm
/assign @alculquicondor

alculquicondor · 2023-06-26T12:19:04Z

pkg/apis/kubeflow/v2beta1/types.go

@@ -154,6 +167,9 @@ type MPIJobSpec struct {
 	// +kubebuilder:default:="/root/.ssh"
 	SSHAuthMountPath string `json:"sshAuthMountPath,omitempty"`

+	// launcherCreationPolicy if WaitForWorkersReady, the launcher is created only after all workers are in Ready state. Defaults to AtStartup.


Add:

// +kubebuilder:validation:Enum:AtStartup;WaitForWorkersReady // +kubebuilder:default:=AtStartup

just below line 170?

Signed-off-by: Lukas Hejtmanek <[email protected]>

alculquicondor · 2023-06-26T17:49:18Z

you need to regenerate the CRDs

Signed-off-by: Lukas Hejtmanek <[email protected]>

alculquicondor · 2023-06-26T17:52:32Z

Thanks, can you squash?

xhejtman · 2023-06-26T17:53:46Z

Thanks, can you squash?

don't know what is it.

tenzen-y · 2023-06-26T17:59:59Z

Thanks, can you squash?

don't know what is it.

That means rebasing and squashing all commits into one.
https://git-scm.com/docs/user-manual#interactive-rebase

xhejtman · 2023-06-26T18:01:25Z

Well, couldn't it be done at your side? I am not skilled with git much, just basic commit/push

tenzen-y · 2023-06-26T18:05:07Z

Well, couldn't it be done at your side? I am not skilled with git much, just basic commit/push

I can do that. But if so, I will become the main committer and you are the co-author. Are you ok?

xhejtman · 2023-06-26T18:10:45Z

Well, couldn't it be done at your side? I am not skilled with git much, just basic commit/push

I can do that. But if so, I will become the main committer and you are the co-author. Are you ok?

No problem at all, I don't need any credit here ;) Thanks!

tenzen-y · 2023-06-26T18:13:11Z

Well, couldn't it be done at your side? I am not skilled with git much, just basic commit/push

I can do that. But if so, I will become the main committer and you are the co-author. Are you ok?

No problem at all, I don't need any credit here ;) Thanks!

OK. Let me try.

alculquicondor · 2023-06-26T18:13:34Z

I think I can force the squash. Let me try

alculquicondor · 2023-06-26T18:16:32Z

nvm

The bot does squash by default.

For example: fda0532

alculquicondor · 2023-06-26T18:16:41Z

/approve

google-oss-prow · 2023-06-26T18:16:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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

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

alculquicondor · 2023-06-26T18:16:54Z

/lgtm

tenzen-y · 2023-06-26T18:19:06Z

nvm

The bot does squash by default.

For example: fda0532

Ah, right.
@xhejtman Thanks for your contribution!
/lgtm

google-oss-prow bot requested review from alculquicondor and gaocegege December 13, 2022 12:01

google-oss-prow bot added the size/M label Dec 13, 2022

alculquicondor reviewed Dec 13, 2022

View reviewed changes

v2/pkg/controller/mpi_job_controller.go Outdated Show resolved Hide resolved

alculquicondor reviewed Dec 19, 2022

View reviewed changes

v2/pkg/controller/mpi_job_controller.go Outdated Show resolved Hide resolved

ahg-g reviewed Dec 22, 2022

View reviewed changes

xhejtman force-pushed the wait branch 2 times, most recently from df327cb to f975ef5 Compare April 4, 2023 21:31

xhejtman closed this Apr 4, 2023

xhejtman force-pushed the wait branch from f975ef5 to 03bba1f Compare April 4, 2023 21:36

google-oss-prow bot added size/XS and removed size/M labels Apr 4, 2023

Real rebase of waitforworkes option

013fa15

Signed-off-by: Lukas Hejtmanek <[email protected]>

xhejtman reopened this Apr 4, 2023

google-oss-prow bot added size/M and removed size/XS labels Apr 4, 2023

xhejtman added 2 commits June 22, 2023 22:50

Swagger fix

bba9b9d

Signed-off-by: Lukas Hejtmanek <[email protected]>

Fix readme

feac4dc

Signed-off-by: Lukas Hejtmanek <[email protected]>

Fix readme again

4488b06

Signed-off-by: Lukas Hejtmanek <[email protected]>

tenzen-y reviewed Jun 23, 2023

View reviewed changes

pkg/apis/kubeflow/v2beta1/types.go Outdated Show resolved Hide resolved

Add comments

62f144b

Signed-off-by: Lukas Hejtmanek <[email protected]>

tenzen-y reviewed Jun 25, 2023

View reviewed changes

google-oss-prow bot assigned tenzen-y Jun 25, 2023

google-oss-prow bot added the lgtm label Jun 25, 2023

alculquicondor reviewed Jun 26, 2023

View reviewed changes

Add kubebuilder annotations

d1f814f

Signed-off-by: Lukas Hejtmanek <[email protected]>

google-oss-prow bot removed the lgtm label Jun 26, 2023

Fix manifests

0f25f0b

Signed-off-by: Lukas Hejtmanek <[email protected]>

google-oss-prow bot added the approved label Jun 26, 2023

google-oss-prow bot added the lgtm label Jun 26, 2023

google-oss-prow bot merged commit f8d815c into kubeflow:master Jun 26, 2023

	// Spawn launcher after all workers are in Ready state if true
	// WaitForWorkers if true, the launcher is created only after all workers are in Ready state

Run workers first and wait for them #484

Run workers first and wait for them #484

Conversation

xhejtman commented Dec 13, 2022

alculquicondor commented Dec 13, 2022

alculquicondor commented Dec 19, 2022

xhejtman commented Dec 19, 2022

alculquicondor commented Dec 19, 2022

terrytangyuan commented Dec 20, 2022

alculquicondor commented Dec 20, 2022

ahg-g 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

xhejtman commented Jan 11, 2023

tenzen-y commented Feb 22, 2023

alculquicondor commented Apr 4, 2023

tenzen-y commented Apr 4, 2023

xhejtman commented Apr 4, 2023

alculquicondor commented Apr 4, 2023

xhejtman commented Apr 4, 2023

xhejtman commented Apr 4, 2023

alculquicondor commented Jun 22, 2023

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y 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

alculquicondor commented Jun 26, 2023

alculquicondor commented Jun 26, 2023

xhejtman commented Jun 26, 2023

tenzen-y commented Jun 26, 2023

xhejtman commented Jun 26, 2023

tenzen-y commented Jun 26, 2023

xhejtman commented Jun 26, 2023

tenzen-y commented Jun 26, 2023

alculquicondor commented Jun 26, 2023

alculquicondor commented Jun 26, 2023

alculquicondor commented Jun 26, 2023

google-oss-prow bot commented Jun 26, 2023

alculquicondor commented Jun 26, 2023

tenzen-y commented Jun 26, 2023