Reorganize controllers code structure #1302

Jeffwan · 2021-07-14T22:19:18Z

This PR is part of #1299 kubeflow/common#138

It resolves "3. Simplify controller code structures. Merge pods.go service.go job.go into one file which is the core JobController implementation."

Major changes

Fix crd installation issue. OpenAPI v3.0 validation schema doesn't allow fields other than metadata.name and metadata.generateName if metada is specified. (https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema)
Embed common.JobController instead of TFController in TFJobReconciler. The major motivation is

TFController implementation still uses low level tfjob clients which should be deprecated
We need another migration at the time we cleanup TFController codes.

Reorganize Controllers code structures to make them clean. After the clean up, using pytorch as an example, pytorch_controller.go has all codes of its controller implementation. Language specific setting will be in pytorch.go.

Note: tensorflow is an exception, I didn't clean up codes because we want to keep them for compatibility.


pkg/controller.v1
├── mxnet
│   ├── mxjob_controller.go
│   ├── mxnet.go
│   └── suite_test.go
├── pytorch
│   ├── pytorch.go
│   ├── pytorchjob_controller.go
│   └── suite_test.go
├── tensorflow
│   ├── controller.go
│   ├── controller_test.go
│   ├── informer.go
│   ├── job.go
│   ├── job_test.go
│   ├── pod.go
│   ├── pod_test.go
│   ├── status.go
│   ├── status_test.go
│   ├── suite_test.go
│   ├── tensorflow.go
│   ├── tensorflow_test.go
│   ├── tfjob_controller.go
│   ├── util.go
│   └── util_test.go
└── xgboost
    ├── suite_test.go
    ├── xgboost.go
    └── xgboostjob_controller.go

/cc @kubeflow/wg-training-leads @zw0610

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]>

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]>

Jeffwan · 2021-07-14T22:20:07Z

/hold

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]>

gaocegege · 2021-07-15T02:08:55Z

Thanks for your contribution! 🎉 👍

Is it ready to review?

Jeffwan · 2021-07-15T02:13:06Z

Thanks for your contribution! 🎉 👍

Is it ready to review?

@gaocegege It is. I am running some coarse-grained testing and leave /hold just in case I need more changes

Jeffwan · 2021-07-20T00:43:02Z

/hold cancel

johnugeorge · 2021-07-20T06:19:50Z

pkg/controller.v1/mxnet/mxjob_controller.go

-	}
+	// setup FieldIndexer to inform the manager that this controller owns pods and services,
+	// so that it will automatically call Reconcile on the underlying MXJob when a Pod or Service changes, is deleted, etc.
+	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &corev1.Pod{}, jobOwnerKey, func(rawObj client.Object) []string {


what is the difference of this from earlier watch?

@johnugeorge c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: r}) creates a separate controller and it is still a low level concepts which leverages "sigs.k8s.io/controller-runtime/pkg/controller" in v1. ctrl.Manager facing programing is higher level and use doesn't need handles things like predicate funds.

I think both way work and this is just kubebuilder v2-v3 fashion.

Can we use controller.Watch?

@gaocegege do you mean you preferred to use controller directly with some event handlers?

Technically, ctrl.NewControllerManagedBy(mgr).....Complete(r) will create the controller eventually.
https://github.com/kubernetes-sigs/controller-runtime/blob/1e4d87c9f9e15e4a58bb81909dd787f30ede7693/pkg/builder/controller.go#L169

https://github.com/kubernetes-sigs/controller-runtime/blob/1e4d87c9f9e15e4a58bb81909dd787f30ede7693/pkg/builder/controller.go#L190

https://github.com/kubernetes-sigs/controller-runtime/blob/1e4d87c9f9e15e4a58bb81909dd787f30ede7693/pkg/builder/controller.go#L314

I am ok with either way, Kubebuilder v1 does use controller way https://book-v1.book.kubebuilder.io/basics/simple_controller.html . In kubebuilder v2 and v3, this has been changed. I am running more tests if high level can meet all the cases. If not, we can fallback to lower level. What do you think?

Thanks, I am just curious. Both way LGTM.

@gaocegege

It's pretty buggy using kubebuilder v3 + common (with many expectation operations). I think it's better to change back to kubebuilder v1 way to manually control the pods events.

gaocegege · 2021-07-22T02:52:06Z

LGTM

terrytangyuan

Thanks!

/lgtm

terrytangyuan · 2021-07-22T08:34:03Z

pkg/common/util/client.go

 	if h := os.Getenv("HOME"); h != "" {
 		return h
 	}
 	return os.Getenv("USERPROFILE") // windows
 }
+
+// TODO (Jeffwan@): Find an elegant way to either use delegatingReader or directly use clientss


Suggested change

// TODO (Jeffwan@): Find an elegant way to either use delegatingReader or directly use clientss

// TODO (Jeffwan@): Find an elegant way to either use delegatingReader or directly use clients

Nice catch. I will address all grammar issue in one PR.

Jeffwan · 2021-07-23T00:52:08Z

/approve

google-oss-robot · 2021-07-23T00:52:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan, 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~~ [Jeffwan]

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

* 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]>

Jeffwan added 2 commits July 14, 2021 13:57

google-oss-robot requested review from zw0610 and a team July 14, 2021 22:19

google-oss-robot added the size/XXL label Jul 14, 2021

google-oss-robot added the do-not-merge/hold label Jul 14, 2021

Jeffwan force-pushed the cleanup_controllers_new branch from db91333 to 4902eca Compare July 14, 2021 22:23

Jeffwan force-pushed the cleanup_controllers_new branch from 4902eca to 22cc34e Compare July 14, 2021 22:24

google-oss-robot removed the do-not-merge/hold label Jul 20, 2021

johnugeorge reviewed Jul 20, 2021

View reviewed changes

terrytangyuan approved these changes Jul 22, 2021

View reviewed changes

google-oss-robot assigned terrytangyuan Jul 22, 2021

google-oss-robot added the lgtm label Jul 22, 2021

google-oss-robot added the approved label Jul 23, 2021

google-oss-robot merged commit b8d5d3c into kubeflow:all-in-one-operator Jul 23, 2021

Jeffwan deleted the cleanup_controllers_new branch July 23, 2021 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize controllers code structure #1302

Reorganize controllers code structure #1302

Jeffwan commented Jul 14, 2021 •

edited

Loading

Jeffwan commented Jul 14, 2021

gaocegege commented Jul 15, 2021

Jeffwan commented Jul 15, 2021

Jeffwan commented Jul 20, 2021

johnugeorge Jul 20, 2021

Jeffwan Jul 21, 2021 •

edited

Loading

gaocegege Jul 21, 2021

Jeffwan Jul 21, 2021

gaocegege Jul 22, 2021

Jeffwan Jul 29, 2021

gaocegege commented Jul 22, 2021

terrytangyuan left a comment

terrytangyuan Jul 22, 2021

Jeffwan Jul 23, 2021

Jeffwan commented Jul 23, 2021

google-oss-robot commented Jul 23, 2021

	// TODO (Jeffwan@): Find an elegant way to either use delegatingReader or directly use clientss
	// TODO (Jeffwan@): Find an elegant way to either use delegatingReader or directly use clients

Reorganize controllers code structure #1302

Reorganize controllers code structure #1302

Conversation

Jeffwan commented Jul 14, 2021 • edited Loading

Jeffwan commented Jul 14, 2021

gaocegege commented Jul 15, 2021

Jeffwan commented Jul 15, 2021

Jeffwan commented Jul 20, 2021

johnugeorge Jul 20, 2021

Choose a reason for hiding this comment

Jeffwan Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

gaocegege Jul 21, 2021

Choose a reason for hiding this comment

Jeffwan Jul 21, 2021

Choose a reason for hiding this comment

gaocegege Jul 22, 2021

Choose a reason for hiding this comment

Jeffwan Jul 29, 2021

Choose a reason for hiding this comment

gaocegege commented Jul 22, 2021

terrytangyuan left a comment

Choose a reason for hiding this comment

terrytangyuan Jul 22, 2021

Choose a reason for hiding this comment

Jeffwan Jul 23, 2021

Choose a reason for hiding this comment

Jeffwan commented Jul 23, 2021

google-oss-robot commented Jul 23, 2021

Jeffwan commented Jul 14, 2021 •

edited

Loading

Jeffwan Jul 21, 2021 •

edited

Loading