fix https://github.com/kubeflow/training-operator/issues/1706 #1707

HeGaoYuan · 2022-12-23T05:45:01Z

Which issue(s) this PR fixes : #1706

Checklist:

Docs included if any changes are user facing

johnugeorge · 2022-12-23T06:34:37Z

cmd/training-operator.v1/main.go

@@ -74,6 +75,7 @@ func main() {
 		"If set, it only monitors kubeflow jobs in the given namespace.")
 	flag.IntVar(&monitoringPort, "monitoring-port", 9443, "Endpoint port for displaying monitoring metrics. "+
 		"It can be set to \"0\" to disable the metrics serving.")
+	flag.IntVar(&controllerThreads, "controller-threads", 10, "Number of worker threads used by the controller.")


Can we keep current defaults here(1)?

Most users are too "lazy" to change the default values, or they wish the default values can handle most situations, I think the default value set to 10 is better?

This value is highly dependent on deployment environment and user needs. I would recommend to keep it default 1 and user can override it based on their needs. (Ref: https://docs.okd.io/latest/operators/operator_sdk/golang/osdk-golang-tutorial.html#osdk-golang-controller-configs_osdk-golang-tutorial)

@HeGaoYuan I agree with @johnugeorge. We should keep it default 1 since it would be best to set values as the minimal requirement one as default.

So far I am still stick on my opinion.

spark-on-k8s-operator is a great project which is similar to training-operator. It use the default value 10 for controllerThreads.( https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/c261df66a00635509f7d8cb2a7fba4c602c9228e/main.go#L56 )

And I think the robustness is more important than minimal requirement for a production-grade project.

Welcome more programmers to convince me of 1 is better than 10. 😁😁

I was totally agree that the default value should be 1 here, regards as following reasons and more,

the default value of controller.Options.MaxConcurrentReconciles is 1, cf

if we do not make sure that EVERYTHING now and future are completely thread safe, it's better to keep the minimal working configuration as default.

Anyway, keep default as DEFAULT would be better in my personal point of view.

@HeGaoYuan Can you update the PR?

@HeGaoYuan Can you update the PR?

@johnugeorge done

coveralls · 2022-12-23T06:39:55Z

Pull Request Test Coverage Report for Build 3781033970

12 of 30 (40.0%) changed or added relevant lines in 7 files are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.05%) to 39.243%

Changes Missing Coverage	Changed/Added Lines	%
pkg/controller.v1/mxnet/mxjob_controller.go	3	0.0%
pkg/controller.v1/xgboost/xgboostjob_controller.go	3	0.0%
pkg/controller.v1/register_controller.go	12	0.0%

Files with Coverage Reduction	New Missed Lines	%
pkg/controller.v1/register_controller.go	1	50.0%
pkg/controller.v1/mpi/mpijob_controller.go	2	77.46%

Totals
Change from base Build 3759228064:	0.05%
Covered Lines:	2663
Relevant Lines:	6786

💛 - Coveralls

johnugeorge · 2022-12-23T07:11:45Z

Related: #1708

tenzen-y · 2022-12-23T08:01:28Z

cmd/training-operator.v1/main.go

@@ -74,6 +75,7 @@ func main() {
 		"If set, it only monitors kubeflow jobs in the given namespace.")
 	flag.IntVar(&monitoringPort, "monitoring-port", 9443, "Endpoint port for displaying monitoring metrics. "+
 		"It can be set to \"0\" to disable the metrics serving.")
+	flag.IntVar(&controllerThreads, "controller-threads", 10, "Number of worker threads used by the controller.")


Should we set the controllerThreads per each framework since I don't think users create many Jobs using all frameworks simultaneously?

This means we introduce --tfjob-threads, pytorchjob-threads, and so on here instead of the --controller-threads.

@johnugeorge wdyt?

Users now can enable and disable schemes that supported by the operator instance.

I think we can keep the command line options simple at first and update it to a more sophisticated version when real cases pump in.

@zw0610 I see; we can select schemes in the following. Thanks for clarifying!

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

Lines 69 to 70 in 69813fb

flag.Var(&enabledSchemes, "enable-scheme", "Enable scheme(s) as --enable-scheme=tfjob --enable-scheme=pytorchjob, case insensitive."+

" Now supporting TFJob, PyTorchJob, MXNetJob, XGBoostJob, PaddleJob. By default, all supported schemes will be enabled.")

kuizhiqing · 2022-12-26T02:50:17Z

pkg/controller.v1/mpi/mpijob_controller.go

@@ -176,9 +176,10 @@ func (jc *MPIJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
 }

 // SetupWithManager sets up the controller with the Manager.
-func (jc *MPIJobReconciler) SetupWithManager(mgr ctrl.Manager) error {
+func (jc *MPIJobReconciler) SetupWithManager(mgr ctrl.Manager, controllerThreads int) error {


Can we or should we introduce a way to keep the SetupWithManager API, may be extending the configuration struct is better than extending/changing API.

Is that make sense to introduce MaxConcurrentReconciles config to common.JobController ?

What's your opinion? @johnugeorge

@kuizhiqing Yes. But I think, we need more refactoring. eg: NewReconciler can take common.JobControllerConfiguration as argument and we can add MaxConcurrentReconciles to common.JobControllerConfiguration.

We can do this in a separate PR as well.

@johnugeorge yes, I agree with that.

johnugeorge · 2022-12-26T18:50:52Z

Thanks @HeGaoYuan

/lgtm
/approve

google-oss-prow · 2022-12-26T18:50:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HeGaoYuan, johnugeorge

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

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

* Update sdk to v1.6.0rc0 * Update sdk to v1.6.0rc0 * Update sdk to v1.6.0rc0 * Update to latest Image * [Bugfix][SDK] pod has no metadata attr anymore in the get_job_logs() … (#1760) --------- Co-authored-by: Hongzhi Chen <chzyaobaiwei@gmail.com>

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

google-oss-prow bot requested review from jinchihe and kuizhiqing December 23, 2022 05:45

johnugeorge reviewed Dec 23, 2022

View reviewed changes

tenzen-y reviewed Dec 23, 2022

View reviewed changes

kuizhiqing reviewed Dec 26, 2022

View reviewed changes

fix kubeflow#1706

600dffe

HeGaoYuan force-pushed the issues1706 branch from e701c9a to 600dffe Compare December 26, 2022 11:45

google-oss-prow bot assigned johnugeorge Dec 26, 2022

google-oss-prow bot added the lgtm label Dec 26, 2022

google-oss-prow bot added the approved label Dec 26, 2022

google-oss-prow bot merged commit e13d262 into kubeflow:master Dec 26, 2022

This was referenced Jan 15, 2023

Add MaxConcurrentReconciles to JobControllerConfiguration kubeflow/common#204

Closed

Add MaxConcurrentReconciles to JobControllerConfiguration kubeflow/common#205

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix https://github.com/kubeflow/training-operator/issues/1706 #1707

fix https://github.com/kubeflow/training-operator/issues/1706 #1707

HeGaoYuan commented Dec 23, 2022

johnugeorge Dec 23, 2022

HeGaoYuan Dec 23, 2022

johnugeorge Dec 23, 2022

tenzen-y Dec 23, 2022

HeGaoYuan Dec 23, 2022 •

edited

kuizhiqing Dec 26, 2022

johnugeorge Dec 26, 2022

HeGaoYuan Dec 26, 2022 •

edited

coveralls commented Dec 23, 2022 •

edited

johnugeorge commented Dec 23, 2022

tenzen-y Dec 23, 2022

zw0610 Dec 23, 2022

tenzen-y Dec 23, 2022

kuizhiqing Dec 26, 2022

johnugeorge Dec 26, 2022

kuizhiqing Dec 26, 2022

johnugeorge commented Dec 26, 2022

google-oss-prow bot commented Dec 26, 2022

	flag.Var(&enabledSchemes, "enable-scheme", "Enable scheme(s) as --enable-scheme=tfjob --enable-scheme=pytorchjob, case insensitive."+
	" Now supporting TFJob, PyTorchJob, MXNetJob, XGBoostJob, PaddleJob. By default, all supported schemes will be enabled.")

fix https://github.com/kubeflow/training-operator/issues/1706 #1707

fix https://github.com/kubeflow/training-operator/issues/1706 #1707

Conversation

HeGaoYuan commented Dec 23, 2022

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

HeGaoYuan Dec 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeGaoYuan Dec 26, 2022 • edited

Choose a reason for hiding this comment

coveralls commented Dec 23, 2022 • edited

Pull Request Test Coverage Report for Build 3781033970

💛 - Coveralls

johnugeorge commented Dec 23, 2022

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

johnugeorge commented Dec 26, 2022

google-oss-prow bot commented Dec 26, 2022

HeGaoYuan Dec 23, 2022 •

edited

HeGaoYuan Dec 26, 2022 •

edited

coveralls commented Dec 23, 2022 •

edited