All in one operator rebase merge to master #1320

Jeffwan · 2021-08-02T20:41:45Z

Phase I (#1298 ) and Phase II (#1299 ) work are done and all-in-one-operator branch pass e2e test.
Original tensorflow controller can coexist with refactored operator.

Since we won't have massive refactor work. I think it's a good time to merge back to master.
Phase III work can be done against master laster #1318

What do you think?

/cc @kubeflow/wg-training-leads

* Resolve GroupVersion redeclared issue `go build` will fail since same group version is redeclared in groupversion_info.go Signed-off-by: Jiaxin Shan <[email protected]> * Generate tfjob 1.19.x compatible clientset 1. Update dependencies to 1.19.x compatible 2. Update update-codegen.sh to use pkg from go modules pkg 3. Generate clientsets which supports context Signed-off-by: Jiaxin Shan <[email protected]>

Signed-off-by: Jiaxin Shan <[email protected]>

* Add XGBoost controller 1. Move major controller logics from kubeflow/xgboost-operator to this repo. 2. Adapt to kubebuilder 3.0.0 change 3. Add xgboost train example 4. Leave some TODOs for code refactor later Signed-off-by: Jiaxin Shan <[email protected]> * Move examples from config to example/ folder

* add tensorflow api * check in tensorflow reconciler fix issues from comment

- Generate MXJob api and manifests - Rewrite MXNet controller logic in reconciler pattern Signed-off-by: Jiaxin Shan <[email protected]>

* Remove duplicate FakeWorkQueue Signed-off-by: Jiaxin Shan <[email protected]> * Extract utility to common Move ConvertPodList, ConvertServiceList to common library Signed-off-by: Jiaxin Shan <[email protected]> * Move gang scheduing setting to common Signed-off-by: Jiaxin Shan <[email protected]> * Extract OnDependentCreate/Delete handler to utils Refactor follow methods to job independent and move to common utils - OnDependentCreateFunc - OnDependentDeleteFunc Signed-off-by: Jiaxin Shan <[email protected]> * Refactor SatisfiedExpectations to a common method Signed-off-by: Jiaxin Shan <[email protected]>

* Add docs, register in job apis - Clean up register.go and use reference in groupversion_info.go (generated by kubebuilder) instead - Add doc.go and register.go for framework missing them - Remove global tag “+k8s:deepcopy-gen=package,register” and this should be taken care of by controller-gen. Signed-off-by: Jiaxin Shan <[email protected]> * Generate defaults and openapi for all frameworks 1. Update codegen scripts to generate defaulters and openapi spec for all frameworks 2. Create defaults.go for framework which miss it. Signed-off-by: Jiaxin Shan <[email protected]> * Add Kind in constants.go Signed-off-by: Jiaxin Shan <[email protected]> * Update update-codegen to generate openapi-gen Signed-off-by: Jiaxin Shan <[email protected]> * Run goimports to format the files * Add Plural and Singular to apis constants.go Signed-off-by: Jiaxin Shan <[email protected]>

* Fix crd installation issue CustomResourceDefinition.apiextensions.k8s.io "tfjobs.kubeflow.org" is invalid: spec.validation.openAPIV3Schema.properties[metadata]: Forbidden: must not specify anything other than name and generateName, but metadata is implicitly specified Signed-off-by: Jiaxin Shan <[email protected]> * Clean up controllers 1. Move all ControllerInterface implementations to [framework]_controller.go 2. Clean up job.go pod.go service.go status.go 3. Follow exact same method order here https://github.com/kubeflow/common/blob/9ffa565bc60e08936f7f80cb3f491cf53f256e7f/pkg/apis/common/v1/interface.go Signed-off-by: Jiaxin Shan <[email protected]> * Embed common.JobController instead of TFController I notice previous change embed TFController which means most of the methods still use tfjob client. We need to make controller consistent and `TFJobReconciler` should override `common.JobController`. With this change, it would be very easy to make original tf operator and new reconciler work together. We also avoid one more refactor when we will remove original TFController codes Signed-off-by: Jiaxin Shan <[email protected]>

* Use low-level controller and handlers in SetupWithManager Signed-off-by: Jiaxin Shan <[email protected]> * Add job validation in reconcile loop Signed-off-by: Jiaxin Shan <[email protected]> * Set defaults in onOwnerCreateFunc Signed-off-by: Jiaxin Shan <[email protected]> * Correctly update job status in apiserver Signed-off-by: Jiaxin Shan <[email protected]> * Remove Flag for Kubeconfig to fix flag redefined CI reports `flag redefined: kubeconfig` issue and this is due to duplicate flag registration. See #1316 for more details. Signed-off-by: Jiaxin Shan <[email protected]> * Fix tensorflow job missing default port issue This is to fix original controller issue. It leverages init to register default to scheme. In our unversal operator project, we did clean up for register.go and break the case. For more details, please check #1317 (comment) Signed-off-by: Jiaxin Shan <[email protected]>

google-cla · 2021-08-02T20:41:53Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Jeffwan · 2021-08-02T20:42:15Z

/hold

Please do not cancel hold. I will rebase merge it later instead of using bot. The major reason is we want to reserve the change history and don't want to squash all commits into a huge one.

Jeffwan · 2021-08-02T20:44:43Z

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

@zw0610 Can you help on this?

google-cla · 2021-08-02T20:44:51Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

terrytangyuan

LGTM and +1 to merge manually

google-oss-robot · 2021-08-03T02:17:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: terrytangyuan
To complete the pull request process, please assign johnugeorge after the PR has been reviewed.
You can assign the PR to them by writing /assign @johnugeorge in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

zw0610 · 2021-08-03T05:29:58Z

@googlebot I consent.

config/rbac/tfjob_editor_role.yaml

gaocegege · 2021-08-03T06:29:09Z

/cc @kubeflow/wg-automl-leads

Jeffwan · 2021-08-03T06:46:16Z

Note, we don't bring new changes to master. All the commits in this PR come from @zw0610 and me and they have been approved and merged in all-in-one-operator branch. If you think this change is too big, feel free to click original PR link to check background. This may help you expedite the review process.

gaocegege · 2021-08-03T06:50:30Z

SGTM

Jeffwan · 2021-08-03T16:35:04Z

/cc @johnugeorge @andreyvelich any comments?

andreyvelich · 2021-08-03T20:03:12Z

Sounds good for the changes @Jeffwan.

Original tensorflow controller can coexist with refactored operator.

Since we are making changes for the tf controller, do we still need to build separate image for tf-operator controller or we will use common kubeflow-training image once we merge this PR ?

andreyvelich · 2021-08-03T20:13:14Z

Makefile

+run: manifests generate fmt vet ## Run a controller from your host.
+	go run ./main.go
+
+docker-build: test ## Build docker image with the manager.


Where will we store Dockerfile for the training-operator image ?

#1325 ./build/images/training-operator/Dockerfile

Maybe we should merge that PR first

@Jeffwan Does it look good to create separate dir only for one Dockerfile or we are planing to have some other Dockerfiles under ./build/image ?
For examples we store Dockerfile in the same dir where example is located: https://github.com/kubeflow/tf-operator/tree/8430a040ad38f3ea1f2fd4b3f077d0871b4cdd52/examples/tf_sample.

You mean something like Dockerfile.tf and Dockerfile.training in the same folder? I think that make sense. I can document this change and make some changes later.

Yes, we can do that.
Or we can move Dockerfile to cmd/training-operator.v1/Dockerfile similar to main.go.

Got your point. Currently, I follow existing way to organize the files. I think these be changes for sure. Originally, we put Dockerfile and main.go in separate folders. We can consider to put them together later. I create an improvement issue to track it. #1333

build └── images ├── tf_operator │ └── Dockerfile cmd ├── tf-operator.v1 │ ├── app │ │ ├── options │ │ │ └── options.go │ │ └── server.go │ └── main.go

Thanks @Jeffwan!

Makefile

andreyvelich · 2021-08-03T20:26:52Z

main.go

@@ -0,0 +1,123 @@
+/*


I think we can move this file to cmd/main.go within Dockerfile in the future.
What do you think @Jeffwan ?

Sounds good. cmd/training-operator.v1/main.go

Hi Andrey, I include this change in a separate PR. This PR includes most changes we made in last two stages. There's follow up PR coming to address these suggestions.

pkg/apis/mxnet/v1/groupversion_info.go

andreyvelich · 2021-08-03T20:31:19Z

pkg/client/clientset/versioned/clientset.go

@@ -57,7 +57,7 @@ func NewForConfig(c *rest.Config) (*Clientset, error) {
 	configShallowCopy := *c


We need to remove clientset in the future once we finish implementation.

This need more discussion. Do we want this repo to host all client sets or a separate repo?

We are not sure which project interact with training crds. If there's any, we should prioritize the story

Sounds good, let's track it somewhere.

I create an issue here #1332 to track this discussion

Jeffwan · 2021-08-05T18:56:43Z

I think I addressed all the concerns and suggestions. Some of the improvements are not made in this PR. This PR won't include any direct changes. This is a rebase change from all-in-one-operator to master. Any changes should be made against all-one-one-operator branch or master directly after it merged

For p1 issues, changes are included in #1325.
For p2 issues, I create issues and they will resolve it later.

This is a clean PR to make original test work. It's safe to merge it separately.

Jeffwan · 2021-08-05T18:57:40Z

/hold cancel

zw0610 and others added 12 commits July 5, 2021 11:53

Add XGBoost API (#1286)

76114c1

Temporarily add Jeffwan@ to OWNERS (#1287)

a59f3be

Fix all client request that needs contexts (#1292)

d72641c

Signed-off-by: Jiaxin Shan <[email protected]>

add pytorch API and controller (#1294)

c172880

add tf reconciler to the unified operator (#1295)

91ae0a8

* add tensorflow api * check in tensorflow reconciler fix issues from comment

Add MXJob v1 api and controller (#1296)

9e7e09f

- Generate MXJob api and manifests - Rewrite MXNet controller logic in reconciler pattern Signed-off-by: Jiaxin Shan <[email protected]>

google-oss-robot requested a review from a team August 2, 2021 20:41

google-oss-robot added the size/XXL label Aug 2, 2021

google-oss-robot added the do-not-merge/hold label Aug 2, 2021

Jeffwan requested a review from zw0610 August 2, 2021 20:42

Jeffwan mentioned this pull request Aug 2, 2021

Cut 1.2.0 tag and release a stable version of master #1321

Closed

terrytangyuan approved these changes Aug 3, 2021

View reviewed changes

zw0610 reviewed Aug 3, 2021

View reviewed changes

config/rbac/tfjob_editor_role.yaml Show resolved Hide resolved

andreyvelich reviewed Aug 3, 2021

View reviewed changes

Jeffwan mentioned this pull request Aug 4, 2021

Should training operator keep clientset in the same repository? #1332

Closed

Remove inline runPolicy and register defaulter (#1330)

bac8320

Jeffwan mentioned this pull request Aug 5, 2021

Best practice to organize main.go and Dockerfile? #1333

Closed

google-oss-robot removed the do-not-merge/hold label Aug 5, 2021

Jeffwan merged commit 2b255d2 into master Aug 5, 2021

Jeffwan deleted the all-in-one-operator branch August 13, 2021 18:36

Jeffwan mentioned this pull request Aug 13, 2021

Training WG and Kubeflow 1.4 release kubeflow/manifests#1962

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

All in one operator rebase merge to master #1320

All in one operator rebase merge to master #1320

Jeffwan commented Aug 2, 2021

google-cla bot commented Aug 2, 2021

Jeffwan commented Aug 2, 2021 •

edited

Loading

Jeffwan commented Aug 2, 2021

google-cla bot commented Aug 2, 2021

terrytangyuan left a comment

google-oss-robot commented Aug 3, 2021

zw0610 commented Aug 3, 2021

gaocegege commented Aug 3, 2021

Jeffwan commented Aug 3, 2021 •

edited

Loading

gaocegege commented Aug 3, 2021

Jeffwan commented Aug 3, 2021

andreyvelich commented Aug 3, 2021

andreyvelich Aug 3, 2021 •

edited

Loading

Jeffwan Aug 3, 2021

andreyvelich Aug 3, 2021

Jeffwan Aug 3, 2021

andreyvelich Aug 3, 2021

Jeffwan Aug 5, 2021 •

edited

Loading

andreyvelich Aug 5, 2021

andreyvelich Aug 3, 2021

Jeffwan Aug 3, 2021

Jeffwan Aug 5, 2021

andreyvelich Aug 3, 2021

Jeffwan Aug 3, 2021

andreyvelich Aug 3, 2021

Jeffwan Aug 4, 2021

Jeffwan commented Aug 5, 2021 •

edited

Loading

Jeffwan commented Aug 5, 2021

		@@ -57,7 +57,7 @@ func NewForConfig(c rest.Config) (Clientset, error) {
		configShallowCopy := *c

All in one operator rebase merge to master #1320

All in one operator rebase merge to master #1320

Conversation

Jeffwan commented Aug 2, 2021

google-cla bot commented Aug 2, 2021

Jeffwan commented Aug 2, 2021 • edited Loading

Jeffwan commented Aug 2, 2021

google-cla bot commented Aug 2, 2021

terrytangyuan left a comment

Choose a reason for hiding this comment

google-oss-robot commented Aug 3, 2021

zw0610 commented Aug 3, 2021

gaocegege commented Aug 3, 2021

Jeffwan commented Aug 3, 2021 • edited Loading

gaocegege commented Aug 3, 2021

Jeffwan commented Aug 3, 2021

andreyvelich commented Aug 3, 2021

andreyvelich Aug 3, 2021 • edited Loading

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

Jeffwan Aug 5, 2021 • edited Loading

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

Choose a reason for hiding this comment

Jeffwan commented Aug 5, 2021 • edited Loading

Jeffwan commented Aug 5, 2021

Jeffwan commented Aug 2, 2021 •

edited

Loading

Jeffwan commented Aug 3, 2021 •

edited

Loading

andreyvelich Aug 3, 2021 •

edited

Loading

Jeffwan Aug 5, 2021 •

edited

Loading

Jeffwan commented Aug 5, 2021 •

edited

Loading