implement trainingjob controller without autoscaler

[m3ngyang]

This pull request implements a training job controller without autoscaler, which registers a self-defined resource called TrainingJob and then watches the event of TrainingJob to manage the lifecycle of TrainingJob instance.

[typhoonzero]

Exported type must have comments. Curious this should be checked by the linter.

[m3ngyang]

Right, I will supplement some comments.

[typhoonzero]

We prefer to use log15 recently.

[m3ngyang]

Is there any difference between those two log packages?

[typhoonzero]

Maybe we need some document or a sample controller to inform these concepts in a controller:

• informer
• event broadcaster
• workqueue
• recorder

[typhoonzero]

Maybe can reduce some info logs, output info log only when the operation success or fail.

[m3ngyang]

Maybe we can use different log levels for them, abundant logs are useful when debugging.

[typhoonzero]

Looks like the name should be processNextWorkItem

[m3ngyang]

Yes, it's a typo here.

[helinwang]

"If a second signal is caught, the program is terminated with exit code 1." --- this is a hardcoded rule that people don't expect in general. Do we really need it? (if the user really want to terminate it, he can send SIGKILL using command kill -9).

For graceful shutdown, I think typically it's done in the following way:

1. setup signal handler
2. start a goroutine that does the real work, block main function
3. when signal is caught, trigger graceful shutdown (e.g., closing a channel, cancel a context).
4. the main function will return normally when graceful shutdown is complete.

An example: https://gist.github.com/peterhellberg/38117e546c217960747aacf689af3dc2

[helinwang]

Please put your name after TODO, e.g., // TODO(helin): ...

[m3ngyang]

This comment is redundant, and I'll delete it. A leader election has been implemented to assure that there is only one controller to manage TrainingJob.

[helinwang]

Do we need this TODO? Maybe it's the job of the binary (e.g., cmd/paddle_controller/paddle_controller.go) to ensure only one instance is running, rather than the library.

[helinwang]

Why do we need a worker pool? Goroutines are already multiplexed onto a thread pool by the Go runtime, perhaps we can have a single loop, and start a new goroutine for each iteration.

[m3ngyang]

Sorry, I don't understand the differences between the worker pool and the single loop. Here, wait.Util(f func(), period time.Duration, stopCh <-chan struct{}) loops until stop channel is closed, running f every period.

[helinwang]

No worries, let me explain.

Here is the current code:

for i := 0; i < threadiness; i++ {
	go wait.Until(c.runWorker, time.Second, stopCh)
}

func (c *TrainingJobController) runWorker() {
	for c.processNextWorkItem() {
	}
}

And there are threadiness number of gorountines running all work items (assume N). The max concurrency is threadiness.

Another implementation is:

go wait.Until(c.runWorker, time.Second, stopCh) // only a single wait.Until

func (c *TrainingJobController) runWorker() {
	for item := range c.itemCh {
                go c.process(item)
	}
}

In this way the The max concurrency is N rather than threadiness. Eliminating the unnecessary configuration value threadiness, at the same time provides better concurrency.

In C++ developers usually uses a thread pool, but in Go, the runtime already use a thread pool to very efficiently run Goroutines, based on a very good scheduling algorithm. We don't have to worry about it.

Does it makes sense to you?

[m3ngyang]

Thanks for your elaborated explanation, and I consulted implementation of some built-in controllers, such as deployment controller and job controller. These controllers usually use a controlled number to indicate the max number of objects that are allowed to sync concurrently, instead of the total number. Besides, sync number can be managed by the configuration of controller-manager, for example --concurrent-deployment-syncs int32.
To sum up, I think it's more reasonable to use threadiness as the max concurrency.

[helinwang]

I see, I did not realize that there is a need for limiting the concurrency, now it makes sense. Thanks!

[m3ngyang]

this pr is included in #24
closed here.