search

kubeflow / common

Common APIs and libraries shared by other Kubeflow operator repositories.
Apache License 2.0
51 stars 73 forks source link

[feature] Add reconciler.v1 package along with controller.v1 #140

Open zw0610 opened 3 years ago

zw0610 commented 3 years ago

The common repo offers a controller.v1 package which is designed for low-level controller mode in kubebuilder. As we are working on a unified controller, it seems a reconciler.v1 package will help future developers to code in high-level reconciler mode.

I would like to take a try if developers think such a reconciler.v1 package will be helpful.

There will be some code overlapping between the controller and reconciler packages, which we can extract into an individual package so that these code can be re-used.

zw0610 commented 3 years ago

/cc @terrytangyuan @Jeffwan

johnugeorge commented 3 years ago

I think, we should move away from low level controller mode so that code is easy to maintain and new devs will find it easy. However, difficulty is to ensure stability of the code base when doing the porting work.

terrytangyuan commented 3 years ago

Agreed. We should only expose modules to developers when needed. Otherwise it might introduce additional maintenance efforts such as backwards compatibility and versioning of the new module.

zw0610 commented 3 years ago

I see. So let me prepare the reconciler.v1 package first. We can further discuss how to deal with the controller mode code.

Jeffwan commented 3 years ago

Current library provides reconcile logics. It's kind of neutral and we did some refactor last year to make it work with both low level controller (tf, mxnet) or high level reconciler (xgboost). It we plan to move to reconciler.v1. Let's add more details what need to do in reconciler.v1. Does it bring locking to specific framework like kubebuilder?

tenzen-y commented 1 year ago

@zw0610 Are there tasks left? We need to determine whether to use controller.v1 or reconciler.v1 when we merge kubeflow/common to kubeflow/training-operator.

/cc @gaocegege @johnugeorge @terrytangyuan

ref: https://github.com/kubeflow/training-operator/issues/1714

zw0610 commented 1 year ago

no. we shall keep controller.v1 in the merge given the minimal changes to the code.

On Wed, Jan 18, 2023 at 04:48 Yuki Iwai @.***> wrote:

@zw0610 https://github.com/zw0610 Are there tasks left? We need to determine whether to use controller.v1 or reconciler.v1 when we merge kubeflow/common to kubeflow/training-operator.

/cc @gaocegege https://github.com/gaocegege @johnugeorge https://github.com/johnugeorge @terrytangyuan https://github.com/terrytangyuan

— Reply to this email directly, view it on GitHub https://github.com/kubeflow/common/issues/140#issuecomment-1386032196, or unsubscribe https://github.com/notifications/unsubscribe-auth/AK7V6IV6AFLMZJQHTW33XR3WS4ATFANCNFSM4762ZUYQ . You are receiving this because you were mentioned.Message ID: @.***>

tenzen-y commented 1 year ago

no. we shall keep controller.v1 in the merge given the minimal changes to the code.

Makes sense. Let's switch to reconciler.v1 after we merge kubeflow/common to kubeflow/training-operator.