add mpi-operator(v1) to the unified operator by hackerboy01 · Pull Request #1457 · kubeflow/trainer

hackerboy01 · 2021-11-02T03:16:40Z

add mpi-operator(v1) to the unified operator

aws-kf-ci-bot · 2021-11-02T03:16:54Z

Hi @hackerboy01. 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.

Details

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 · 2021-11-02T06:14:01Z

/ok-to-test

terrytangyuan

Wha happened to the previous PR for this?

go.mod

gaocegege · 2021-11-03T06:23:36Z

Hi @hackerboy01

Is it ready to review?

zw0610

generally looks great. We might still need tiny fix.

pkg/apis/mxnet/v1/zz_generated.deepcopy.go

zw0610 · 2021-11-08T08:49:49Z

pkg/controller.v1/mpi/mpijob_controller.go

+//+kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups="",resources=serviceaccount,verbs=get;list;watch;create;delete
+//+kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups="rbac.authorization.k8s.io",resources=roles,verbs=get;list;watch;create;delete


To support elastic feature (horovod-elastic), we also need permission to update role: https://github.com/kubeflow/mpi-operator/blob/master/pkg/controllers/v1/mpi_job_controller.go#L777

zw0610 · 2021-11-08T08:59:37Z

pkg/controller.v1/mpi/mpijob_controller.go

+		utilruntime.HandleError(fmt.Errorf("couldn't get jobKey for job object %#v: %v", mpijob, err))
+	}
+	replicaTypes := util.GetReplicaTypes(mpijob.Spec.MPIReplicaSpecs)
+	needReconcile := util.SatisfiedExpectations(r.Expectations, jobKey, replicaTypes)


function SatisfiedExpectations only checks Pod and Service, which is sufficient for other job API. However, I wonder if MPIJob needs a more sophisticated SatisfiedExpectations implementation.

Was this ever checked?

@alculquicondor you mean mpi-operator had not ever checked excpectations?

I mean, did the author check @zw0610's questions?

zw0610 · 2021-11-08T09:05:00Z

pkg/controller.v1/mpi/mpijob_controller.go

+		return err
+	}
+
+	// inject watching for job related service


I think for mpi-controller.v1, there won't be job-related service. Instead, ServiceAccount, Role, RoleBinding and other resource may need be watched.

I don't think the controller works properly without solving Wang's comment.

This is not addressed? @hackerboy01

pkg/controller.v1/mpi/mpijob_controller.go

terrytangyuan

Great work! Let's make sure we don't introduce additional enhancements and keep this PR almost like copy-paste from the existing controller code. This way it's easy to track/review changes.

terrytangyuan · 2021-11-08T14:57:13Z

/cc @alculquicondor

alculquicondor · 2021-11-08T15:32:16Z

can you also move the unit tests please?

gaocegege · 2021-11-09T07:21:27Z

pkg/apis/mpi/validation/validation.go

+				msg := fmt.Sprintf("MPIReplicaSpec is not valid: Image is undefined in the container of %v", rType)
+				return fmt.Errorf(msg)
+			}
+			// if container.Name == mpiv1.DefaultContainerName {


Should we remove these code?

@alculquicondor We will move the unit tests in the next pr.

Please remove this commented code if it's not actually needed.

fix: controller-gen fix：fmt

terrytangyuan · 2021-11-09T13:35:59Z

@alculquicondor We will move the unit tests in the next pr.

@hackerboy01 I think you'll want to include the tests in this PR. Otherwise, we have no way to verify the correctness of the migrated code.

zw0610 · 2021-11-10T01:39:05Z

@alculquicondor We will move the unit tests in the next pr.

@hackerboy01 I think you'll want to include the tests in this PR. Otherwise, we have no way to verify the correctness of the migrated code.

As we changed from the controller mode to reconciler mode, I would suggest only moving unit tests which do not use the old controller directly. Tests with the old controller mode does not verify the correctness of this new reconciler.

For the next pr, we can follow the suite_test.go and tensorflow_test.go and add unit tests for the reconciler.

What do you think? @terrytangyuan

Meanwhile, it seems we also need to add more tests to tensorflow reconciler as well. @Jeffwan

terrytangyuan · 2021-11-10T03:50:20Z

Why adding tests in the next PR instead of having it as part of this PR?

zw0610 · 2021-11-10T03:55:34Z

Why adding tests in the next PR instead of having it as part of this PR?

Because we lack a good example among existing reconcilers. Anyone could add tests for tf/pytorch reconciler so mpi reconciler can follow the path?

alculquicondor · 2021-11-10T14:16:13Z

As we changed from the controller mode to reconciler mode, I would suggest only moving unit tests which do not use the old controller directly.

Yes, the unit tests need to be adapted, just like the code was adapted. But we shouldn't add such a significant amount of code without tests in the same PR. The contributor could decide to leave and then we have untested code to maintain.

coveralls · 2021-11-11T02:50:09Z

Pull Request Test Coverage Report for Build 1460365743

888 of 1149 (77.28%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+16.0%) to 24.149%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller.v1/mpi/mpijob.go	102	108	94.44%
pkg/controller.v1/mpi/mpijob_controller.go	786	1041	75.5%

Totals
Change from base Build 1431720272:	16.0%
Covered Lines:	1199
Relevant Lines:	4965

💛 - Coveralls

zw0610 · 2021-11-15T06:55:08Z

We just converted all test for v1 controller from mpi-operator to this pr.

terrytangyuan · 2021-11-16T01:44:57Z

LGTM. Thank you for adding the tests!

/cc @alculquicondor Would you like to take another look before we merge?

zw0610 · 2021-11-19T03:03:17Z

Can we move this pr forward? @terrytangyuan

terrytangyuan · 2021-11-19T03:05:19Z

/approve
/lgtm

google-oss-prow · 2021-11-19T03:05:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hackerboy01, terrytangyuan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

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

alculquicondor · 2021-11-22T15:07:56Z

Sorry, I was on vacation. I'll take a look regardless and hopefully we can fix any problems in a follow up.

alculquicondor · 2021-11-22T15:48:18Z

@hackerboy01 are you going to be migrating the v2 operator as well?

terrytangyuan · 2021-11-22T15:52:47Z

@hackerboy01 are you going to be migrating the v2 operator as well?

Let's start the discussion on v2 migration in #1479 instead of on this merged PR.

alculquicondor

I don't think I have more capacity to look at the v1 controller changes, but I'm not convinced that this PR went through a proper review cycle. There are a handful of important open questions that were never looked at.

manifests/base/cluster-role.yaml

alculquicondor · 2021-11-22T19:09:10Z

manifests/base/cluster-role.yaml

+  - apiGroups:
+      - policy
+    resources:
+      - poddisruptionbudgets


I don't recall a need for this either

alculquicondor · 2021-11-22T19:13:23Z

pkg/apis/mpi/v1/constants.go

@@ -0,0 +1,38 @@
+// Copyright 2019 The Kubeflow Authors


2021 here and other files

alculquicondor · 2021-11-22T19:22:06Z

pkg/apis/mpi/v1/groupversion_info.go

+// Package v1 contains API Schema definitions for the kubeflow.org v1 API group
+//+kubebuilder:object:generate=true
+//+groupName=kubeflow.org
+package v1


why do we have a doc.go file if there is already this comment here?

alculquicondor · 2021-11-22T19:41:14Z

pkg/controller.v1/mpi/mpijob_controller.go

+		utilruntime.HandleError(fmt.Errorf("couldn't get jobKey for job object %#v: %v", mpijob, err))
+	}
+	replicaTypes := util.GetReplicaTypes(mpijob.Spec.MPIReplicaSpecs)
+	needReconcile := util.SatisfiedExpectations(r.Expectations, jobKey, replicaTypes)


Was this ever checked?

alculquicondor · 2021-11-22T19:43:42Z

pkg/controller.v1/mpi/mpijob_controller.go

+		return err
+	}
+
+	// inject watching for job related service


I don't think the controller works properly without solving Wang's comment.

alculquicondor · 2021-11-22T19:46:26Z

pkg/controller.v1/mpi/mpijob_controller.go

+}
+
+//mpijob not need delete services
+func (r *MPIJobReconciler) DeletePodsAndServices(runPolicy *commonv1.RunPolicy, job interface{}, pods []*corev1.Pod) error {


which services?

alculquicondor · 2021-11-22T19:46:40Z

pkg/controller.v1/mpi/mpijob_controller.go

+// reconcileServices checks and updates services for each given ReplicaSpec.
+// It will requeue the job in case of an error while creating/deleting services.
+// mpijob not need services
+func (jc *MPIJobReconciler) ReconcileServices(


do we need to implement this function?

alculquicondor · 2021-11-22T19:50:10Z

pkg/controller.v1/mpi/mpijob_controller.go

+			return true
+		}
+
+		r.Scheme.Default(mpiJob)


I'm not convinced that this is safe. If a user changes an MPIJob before the controller had a chance to update the job, I think the defaults would be cleared. And maybe there could be a race condition in the client cache, but I'm not sure how the cache is implemented in controller-runtime.

cc @Jeffwan who did the same for tf-operator

@alculquicondor
I did same thing for tf. Currently, it's still using a lightweight solution, the ideal way is to move the logics to webhooks. If user changes the job, the event sequence for the same object is guaranteed but since they are different kind of events and handled by different methods, I would say there's chance to get into the race condition.

T1: onOwnerCreateFunc is revoked and it tries to set default RunPolicy
T2: onUpdate() is revoked and it updates similar fields
T3. In the end, onOwnerCreateFunc set defaults.

Honestly, I have not played with this case. I agree even if we won't update that fast but the case is still there theoretically. Is there a better way to handle this without webhook? Maybe we can move most of logics to Defaulting https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#defaulting

zw0610 · 2021-11-24T07:19:00Z

@hackerboy01 and I have reviewed the comments above. Fixing pull requests will follow this week.

Jeffwan · 2021-11-25T04:04:06Z

manifests/base/cluster-role.yaml

+  - apiGroups:
+      - apiextensions.k8s.io
+    resources:
+      - customresourcedefinitions


Could you remind me what CRD needs to be created?

Uhm... I wouldn't expect the controller to create CRDs

Jeffwan · 2021-11-25T04:04:08Z

manifests/base/cluster-role.yaml

+  - apiGroups:
+      - policy
+    resources:
+      - poddisruptionbudgets


Jeffwan · 2021-11-25T04:08:38Z

pkg/controller.v1/mpi/mpijob_controller.go

+	}
+
+	if err = validation.ValidateV1MpiJobSpec(&mpijob.Spec); err != nil {
+		logger.Info(err.Error(), "MPIJob failed validation", req.NamespacedName.String())


Agree. We return put the object back to the queue. It's worthless to move it forward.
Another thing is I think the original validation logic is kind of outdated. Personally I don't see too many specific fields to validate. With latest controller tools, I think most of these invalid cases would be blocked by CRD validation.

Jeffwan · 2021-11-25T04:26:42Z

pkg/controller.v1/mpi/mpijob_controller.go

+			return true
+		}
+
+		r.Scheme.Default(mpiJob)


@alculquicondor
I did same thing for tf. Currently, it's still using a lightweight solution, the ideal way is to move the logics to webhooks. If user changes the job, the event sequence for the same object is guaranteed but since they are different kind of events and handled by different methods, I would say there's chance to get into the race condition.

T1: onOwnerCreateFunc is revoked and it tries to set default RunPolicy
T2: onUpdate() is revoked and it updates similar fields
T3. In the end, onOwnerCreateFunc set defaults.

Honestly, I have not played with this case. I agree even if we won't update that fast but the case is still there theoretically. Is there a better way to handle this without webhook? Maybe we can move most of logics to Defaulting https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#defaulting

Jeffwan · 2021-11-25T04:28:02Z

@hackerboy01 Great work. Please help address the comments from @alculquicondor @zw0610 and me. Let's create a reliable and stable version for the users.

aws-kf-ci-bot added the needs-ok-to-test label Nov 2, 2021

google-oss-prow bot added the size/XXL label Nov 2, 2021

google-oss-prow bot requested review from jinchihe, johnugeorge and terrytangyuan November 2, 2021 03:16

aws-kf-ci-bot added ok-to-test and removed needs-ok-to-test labels Nov 2, 2021

hackerboy01 force-pushed the test-pr branch 4 times, most recently from 9c76664 to a030ab0 Compare November 2, 2021 09:01

terrytangyuan reviewed Nov 2, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

hackerboy01 force-pushed the test-pr branch 3 times, most recently from 4ff9a74 to 82071c3 Compare November 3, 2021 05:34

zw0610 reviewed Nov 8, 2021

View reviewed changes

hackerboy01 force-pushed the test-pr branch 2 times, most recently from fa323e5 to d57a6e4 Compare November 8, 2021 13:39

terrytangyuan reviewed Nov 8, 2021

View reviewed changes

hackerboy01 force-pushed the test-pr branch 3 times, most recently from 75dfdb3 to da115b2 Compare November 9, 2021 06:36

gaocegege reviewed Nov 9, 2021

View reviewed changes

fix：add mpi-operator to the unified operator

ce8ed51

fix: controller-gen fix：fmt

hackerboy01 force-pushed the test-pr branch from da115b2 to ce8ed51 Compare November 9, 2021 07:25

zw0610 and others added 2 commits November 14, 2021 20:38

add unittest and fix typo in controller.go

f52224c

Merge branch 'zw0610-test-pr' into test-pr

b1d50da

google-oss-prow bot assigned terrytangyuan Nov 19, 2021

google-oss-prow bot added the lgtm label Nov 19, 2021

google-oss-prow bot added the approved label Nov 19, 2021

google-oss-prow bot merged commit 63a6067 into kubeflow:master Nov 19, 2021

terrytangyuan mentioned this pull request Nov 22, 2021

Migrate v2 MPI operator to the unified operator #1479

Closed

alculquicondor reviewed Nov 22, 2021

View reviewed changes

Jeffwan reviewed Nov 25, 2021

View reviewed changes

hackerboy01 deleted the test-pr branch November 25, 2021 09:35

zw0610 mentioned this pull request Nov 26, 2021

add expectation-related functions for other resources used in mpi-controller #1484

Merged

This was referenced Nov 29, 2021

fix comments for mpi-controller #1485

Merged

Add hackerboy01 to the member list kubeflow/internal-acls#513

Merged

tenzen-y mentioned this pull request Dec 23, 2022

Remove mpi-operator v1 from this repository kubeflow/mpi-operator#486

Closed

Conversation

hackerboy01 commented Nov 2, 2021

Uh oh!

aws-kf-ci-bot commented Nov 2, 2021

Uh oh!

gaocegege commented Nov 2, 2021

Uh oh!

terrytangyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaocegege commented Nov 3, 2021

Uh oh!

zw0610 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

terrytangyuan left a comment

Choose a reason for hiding this comment

Uh oh!

terrytangyuan commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alculquicondor commented Nov 8, 2021

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!

terrytangyuan commented Nov 9, 2021

Uh oh!

zw0610 commented Nov 10, 2021

Uh oh!

terrytangyuan commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zw0610 commented Nov 10, 2021

Uh oh!

alculquicondor commented Nov 10, 2021

Uh oh!

coveralls commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1460365743

💛 - Coveralls

Uh oh!

zw0610 commented Nov 15, 2021

Uh oh!

terrytangyuan commented Nov 16, 2021

Uh oh!

zw0610 commented Nov 19, 2021

Uh oh!

terrytangyuan commented Nov 19, 2021

Uh oh!

google-oss-prow bot commented Nov 19, 2021

Uh oh!

alculquicondor commented Nov 22, 2021

Uh oh!

alculquicondor commented Nov 22, 2021

Uh oh!

terrytangyuan commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

terrytangyuan commented Nov 8, 2021 •

edited

Loading

terrytangyuan commented Nov 10, 2021 •

edited

Loading

coveralls commented Nov 11, 2021 •

edited

Loading

terrytangyuan commented Nov 22, 2021 •

edited

Loading