fix controller deadlook when wait dependson task #2898

renwenlong-github · 2023-06-06T06:58:00Z

hwdef

I don't think this is a good fix. If A waits for B, but B needs to pull a large image, it will take a long time, and two minutes is too short. In fact, I don't think setting a timeout is a good solution.

renwenlong-github · 2023-06-07T09:02:32Z

I don't think this is a good fix. If A waits for B, but B needs to pull a large image, it will take a long time, and two minutes is too short. In fact, I don't think setting a timeout is a good solution.

You are right; as long as you are waiting, you will never know how long you should wait;

we should remove the waiting, because the controller can make multiple judgments. In multiple control, create it if the create condition is met. Otherwise, we can do it next time.

hwdef · 2023-06-07T09:39:09Z

Can you explain in detail how controller can make multiple judgments

renwenlong-github · 2023-06-07T09:49:08Z

I don't think this is a good fix. If A waits for B, but B needs to pull a large image, it will take a long time, and two minutes is too short. In fact, I don't think setting a timeout is a good solution.

You are right; as long as you are waiting, you will never know how long you should wait;

we should remove the waiting, because the controller can make multiple judgments. In multiple control, create it if the create condition is met. Otherwise, we can do it next time.

I don't think this is a good fix. If A waits for B, but B needs to pull a large image, it will take a long time, and two minutes is too short. In fact, I don't think setting a timeout is a good solution.

i think bug code here

func (cc *jobcontroller) waitDependsOnTaskMeetCondition(taskName string, taskIndex int, podToCreateEachTask []*v1.Pod, job *batch.Job) {
	if job.Spec.Tasks[taskIndex].DependsOn == nil {
		return
	}

	dependsOn := *job.Spec.Tasks[taskIndex].DependsOn
	if len(dependsOn.Name) > 1 && dependsOn.Iteration == batch.IterationAny {
		wait.PollInfinite(detectionPeriodOfDependsOntask, func() (bool, error) {
			for _, task := range dependsOn.Name {
				if cc.isDependsOnPodsReady(task, job) {
					return true, nil
				}
			}
			return false, nil
		})
	} else {
		for _, dependsOnTask := range dependsOn.Name {
			wait.PollInfinite(detectionPeriodOfDependsOntask, func() (bool, error) {
				if cc.isDependsOnPodsReady(dependsOnTask, job) {
					return true, nil
				}
				return false, nil
			})
		}
	}
}

If the wait.PollInfinite is not terminated, it will wait forever in job preempted

do you think this is a good solution？

func (cc *jobcontroller) waitDependsOnTaskMeetCondition(taskName string, taskIndex int, podToCreateEachTask []*v1.Pod, job *batch.Job) bool {
	if job.Spec.Tasks[taskIndex].DependsOn == nil {
		return true
	}
	dependsOn := *job.Spec.Tasks[taskIndex].DependsOn
	if len(dependsOn.Name) > 1 && dependsOn.Iteration == batch.IterationAny {
		// any ready to create task
		for _, task := range dependsOn.Name {
			if cc.isDependsOnPodsReady(task, job) {
				return true
			}
		}
		return false
	} else {
		for _, dependsOnTask := range dependsOn.Name {
			// any not ready to skip
			if !cc.isDependsOnPodsReady(dependsOnTask, job) {
				return false
			}
			return true
		}
	}
	return true
}

Signed-off-by: Ren <[email protected]>

hwdef · 2023-06-07T11:17:06Z

I probably understand your idea, you want to re-trigger the pod creation through the controller, but are you sure it will re-trigger?

renwenlong-github · 2023-06-08T09:05:35Z

I probably understand your idea, you want to re-trigger the pod creation through the controller, but are you sure it will re-trigger?

I tested it ok, although I don't understand why the volcano doesn't use resync,
Even so, the watch event is enough for the volcano, I will prove my idea through the source code，job controller watch event as follows：

// https://github.com/volcano-sh/volcano/blob/master/pkg/controllers/job/job_controller.go#L155
	cc.jobInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
		AddFunc:    cc.addJob,
		UpdateFunc: cc.updateJob,
		DeleteFunc: cc.deleteJob,
	})
// https://github.com/volcano-sh/volcano/blob/master/pkg/controllers/job/job_controller.go#LL189C6-L189C6
	cc.podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
		AddFunc:    cc.addPod,
		UpdateFunc: cc.updatePod,
		DeleteFunc: cc.deletePod,
	})

A dependsOn B, if B pods no ready, we skip to create A. when pod B ready, job controller add the B event to queue,
it will run SyncJobAction -> SyncJob -> syncJob(jobInfo *apis.JobInfo, updateStatus state.UpdateStatusFn) -> waitDependsOnTaskMeetCondition -> ok -> create A

Maybe, there is something I don't know, but we have tested this fix, It's ok.

If we doesn't fix it, preempt has a bug, example A dependsOn B, A and B was killed all，if controller create B first, it will use a controller work to wait, if 3 work(default workNum) used to wait, the controller will die.
issues #2897

hwdef · 2023-06-09T03:39:57Z

@xiefan-github
Thanks for your test, I will test your submitted code again later, if there is no problem, I will approve this modification.

hwdef

Tested, this code is ok, thanks for your contribution, this fix is awesome!
/approve

hwdef · 2023-06-11T14:27:49Z

@wangyang0616 please trigger the CI

hwdef

/lgtm

Signed-off-by: Ren <[email protected]>

hwdef · 2023-06-14T08:37:19Z

pkg/controllers/job/job_controller_actions.go

@@ -378,7 +377,16 @@ func (cc *jobcontroller) syncJob(jobInfo *apis.JobInfo, updateStatus state.Updat
 go func(taskName string, podToCreateEachTask []*v1.Pod) {
 taskIndex := jobhelpers.GetTasklndexUnderJob(taskName, job)
 if job.Spec.Tasks[taskIndex].DependsOn != nil {
- cc.waitDependsOnTaskMeetCondition(taskName, taskIndex, podToCreateEachTask, job)
+ if !cc.waitDependsOnTaskMeetCondition(taskName, taskIndex, podToCreateEachTask, job) {
+ klog.V(4).Infof("Job %s/%s depends on task not ready", job.Name, job.Namespace)


Please make CI happy,
I think we should improve log level

hwdef · 2023-06-25T07:17:54Z

pkg/controllers/job/job_controller_actions.go

@@ -378,7 +377,16 @@ func (cc *jobcontroller) syncJob(jobInfo *apis.JobInfo, updateStatus state.Updat
 go func(taskName string, podToCreateEachTask []*v1.Pod) {
 taskIndex := jobhelpers.GetTasklndexUnderJob(taskName, job)
 if job.Spec.Tasks[taskIndex].DependsOn != nil {
- cc.waitDependsOnTaskMeetCondition(taskName, taskIndex, podToCreateEachTask, job)
+ if !cc.waitDependsOnTaskMeetCondition(taskName, taskIndex, podToCreateEachTask, job) {
+ klog.Errorf("Job %s/%s depends on task not ready", job.Name, job.Namespace)


Suggested change

klog.Errorf("Job %s/%s depends on task not ready", job.Name, job.Namespace)

klog.V(3).Infof("Job %s/%s depends on task not ready", job.Name, job.Namespace)

Signed-off-by: Ren <[email protected]>

renwenlong-github · 2023-06-25T10:18:59Z

I verify codes success in my local, but it not work in volcano verity, how can I fix it?

hwdef · 2023-06-25T11:29:12Z

It may be an occasional error, but we're not in a hurry to fix it now because spark's CI also has an error (it's not you), we can wait for the spark CI error to be fixed and then retry verify's CI

renwenlong-github · 2023-06-26T03:41:57Z

It may be an occasional error, but we're not in a hurry to fix it now because spark's CI also has an error (it's not you), we can wait for the spark CI error to be fixed and then retry verify's CI

Wait for spark fix, help me to ci, thank you

hwdef

/lgtm

hwdef · 2023-06-28T03:29:56Z

/close

volcano-sh-bot · 2023-06-28T03:30:00Z

@hwdef: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hwdef · 2023-06-28T03:30:09Z

/reopen

volcano-sh-bot · 2023-06-28T03:30:12Z

@hwdef: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hwdef

/approve

william-wang

/lgtm
/approve

volcano-sh-bot · 2023-07-18T06:54:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hwdef, william-wang

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~~ [william-wang]

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

volcano-sh-bot requested review from hzxuzhonghu and qiankunli June 6, 2023 06:58

volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2023

hwdef reviewed Jun 6, 2023

View reviewed changes

renwenlong-github force-pushed the controller-deadlook branch from d9ffb45 to a49af3a Compare June 7, 2023 09:47

renwenlong-github force-pushed the controller-deadlook branch from a49af3a to 96c6acf Compare June 7, 2023 09:54

delete dependson task wait.PollInfinite

1d7c42b

Signed-off-by: Ren <[email protected]>

renwenlong-github force-pushed the controller-deadlook branch from 96c6acf to 1d7c42b Compare June 7, 2023 09:56

renwenlong-github mentioned this pull request Jun 7, 2023

preempt actions causes controller deadlock #2897

Closed

hwdef reviewed Jun 11, 2023

View reviewed changes

volcano-sh-bot assigned hwdef Jun 11, 2023

volcano-sh-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Jun 11, 2023

delete error log

7a9f1ba

Signed-off-by: Ren <[email protected]>

renwenlong-github force-pushed the controller-deadlook branch from 7c40dab to 7a9f1ba Compare June 14, 2023 07:19

hwdef reviewed Jun 14, 2023

View reviewed changes

hwdef reviewed Jun 25, 2023

View reviewed changes

renwenlong-github force-pushed the controller-deadlook branch from 8d3e5fe to e1e2963 Compare June 25, 2023 09:20

change wait dependson job log level to error

6d0ab3b

Signed-off-by: Ren <[email protected]>

renwenlong-github force-pushed the controller-deadlook branch from e1e2963 to 6d0ab3b Compare June 25, 2023 09:26

hwdef reviewed Jun 26, 2023

View reviewed changes

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2023

hwdef mentioned this pull request Jun 26, 2023

optimize: batch create pod with limit on job_controller #2933

Closed

volcano-sh-bot closed this Jun 28, 2023

volcano-sh-bot reopened this Jun 28, 2023

hwdef reviewed Jul 18, 2023

View reviewed changes

william-wang approved these changes Jul 18, 2023

View reviewed changes

volcano-sh-bot assigned william-wang Jul 18, 2023

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2023

volcano-sh-bot merged commit f9d22d6 into volcano-sh:master Jul 18, 2023
20 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix controller deadlook when wait dependson task #2898

fix controller deadlook when wait dependson task #2898

renwenlong-github commented Jun 6, 2023

hwdef left a comment •

edited

Loading

renwenlong-github commented Jun 7, 2023

hwdef commented Jun 7, 2023

renwenlong-github commented Jun 7, 2023 •

edited

Loading

hwdef commented Jun 7, 2023

renwenlong-github commented Jun 8, 2023 •

edited

Loading

hwdef commented Jun 9, 2023

hwdef left a comment

hwdef commented Jun 11, 2023

hwdef left a comment

hwdef Jun 14, 2023

hwdef Jun 25, 2023

renwenlong-github commented Jun 25, 2023

hwdef commented Jun 25, 2023

renwenlong-github commented Jun 26, 2023

hwdef left a comment

hwdef commented Jun 28, 2023

volcano-sh-bot commented Jun 28, 2023

hwdef commented Jun 28, 2023

volcano-sh-bot commented Jun 28, 2023

hwdef left a comment

william-wang left a comment

volcano-sh-bot commented Jul 18, 2023

	klog.Errorf("Job %s/%s depends on task not ready", job.Name, job.Namespace)
	klog.V(3).Infof("Job %s/%s depends on task not ready", job.Name, job.Namespace)

fix controller deadlook when wait dependson task #2898

fix controller deadlook when wait dependson task #2898

Conversation

renwenlong-github commented Jun 6, 2023

hwdef left a comment • edited Loading

Choose a reason for hiding this comment

renwenlong-github commented Jun 7, 2023

hwdef commented Jun 7, 2023

renwenlong-github commented Jun 7, 2023 • edited Loading

hwdef commented Jun 7, 2023

renwenlong-github commented Jun 8, 2023 • edited Loading

hwdef commented Jun 9, 2023

hwdef left a comment

Choose a reason for hiding this comment

hwdef commented Jun 11, 2023

hwdef left a comment

Choose a reason for hiding this comment

hwdef Jun 14, 2023

Choose a reason for hiding this comment

hwdef Jun 25, 2023

Choose a reason for hiding this comment

renwenlong-github commented Jun 25, 2023

hwdef commented Jun 25, 2023

renwenlong-github commented Jun 26, 2023

hwdef left a comment

Choose a reason for hiding this comment

hwdef commented Jun 28, 2023

volcano-sh-bot commented Jun 28, 2023

hwdef commented Jun 28, 2023

volcano-sh-bot commented Jun 28, 2023

hwdef left a comment

Choose a reason for hiding this comment

william-wang left a comment

Choose a reason for hiding this comment

volcano-sh-bot commented Jul 18, 2023

hwdef left a comment •

edited

Loading

renwenlong-github commented Jun 7, 2023 •

edited

Loading

renwenlong-github commented Jun 8, 2023 •

edited

Loading