Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revert cache to original state on evict and bind errors #909

Conversation

@mateuszlitwin
Copy link
Contributor

mateuszlitwin commented Nov 20, 2019

What this PR does / why we need it:
This change ensures that node, task and job info will remain unchanged in case of an error during SchedulerCache.Bind and SchedulerCache.Evict calls.
Before if error occurred during binding phase (e.g. "Selected node NotReady") task could get stuck in the Binding status indefinitely while the real pod would be in the Pending status.

  • SchedulerCache.Evict and SchedulerCache.Bind will revert task status on NodeInfo.UpdateTask and NodeInfo.AddTask errors.
  • Modified behavior of NodeInfo.AddTask. AddTask will now update task's node name upon successful addition, this is similar to how JobInfo.UpdateTaskStatus updates task's status.
  • Handling unchecked error in JobInfo.UpdateTaskStatus.
  • FATAL logging in NodeInfo.UpdateTask when impossible situation happens - failing to add a task after removal of a task from node info.

Might be related to #891.

Special notes for your reviewer:

Release note:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 20, 2019

Hi @mateuszlitwin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot requested review from hex108 and k82cn Nov 20, 2019
@mateuszlitwin mateuszlitwin force-pushed the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch 6 times, most recently from 4fd830d to 272857b Nov 20, 2019
@k82cn

This comment has been minimized.

Copy link
Contributor

k82cn commented Nov 21, 2019

/ok-to-test

@k82cn

This comment has been minimized.

Copy link
Contributor

k82cn commented Nov 21, 2019

Good catch ! Please ping me when it's ready for review :)

@mateuszlitwin

This comment has been minimized.

Copy link
Contributor Author

mateuszlitwin commented Nov 21, 2019

@k82cn Will do.

BTW. The bug that happened on my cluster was that each time "Selected node NotReady" happened during allocation binding, the task would end up in the Binding status even though pod was Pending. This PR is attempt to make Bind/Evict handle failures like that.

However I do not understand why "Selected node NotReady" would happen in the first place. It seems to me it can only happen if task.InitResreq.LessEqual(node.Idle) and node.Idle.Less(task.Resreq). However I would expect that task.Resreq.LessEqual(task.InitResreq) is always true? (https://github.com/kubernetes-sigs/kube-batch/blob/master/pkg/scheduler/actions/allocate/allocate.go#L160; https://github.com/kubernetes-sigs/kube-batch/blob/master/pkg/scheduler/api/node_info.go#L162)

EDIT: I suppose it was some race condition (SchedulerCache and Session having different nodes)

@mateuszlitwin mateuszlitwin force-pushed the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch from 272857b to 6497ba2 Nov 21, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Nov 21, 2019
@mateuszlitwin mateuszlitwin force-pushed the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch from 6497ba2 to 7ba4048 Nov 21, 2019
@mateuszlitwin mateuszlitwin marked this pull request as ready for review Nov 21, 2019
@@ -163,15 +163,18 @@ func (ni *NodeInfo) allocateIdleResource(ti *TaskInfo) error {
ni.Idle.Sub(ti.Resreq)
return nil
}
ni.State = NodeState{

This comment has been minimized.

Copy link
@mateuszlitwin

mateuszlitwin Nov 21, 2019

Author Contributor

Removing this because I do not want to modify node info state on error, also this state might not be true depending on the context.

I think, if it is necessary, this state could be set explicitly after receiving an error from AddTask(), depending on the context in which error occurred.

@mateuszlitwin mateuszlitwin force-pushed the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch 2 times, most recently from 0466973 to bab8cf5 Nov 21, 2019
@mateuszlitwin

This comment has been minimized.

Copy link
Contributor Author

mateuszlitwin commented Nov 21, 2019

@k82cn Ready for review.

@mateuszlitwin mateuszlitwin force-pushed the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch from bab8cf5 to 063b55c Nov 21, 2019
This change ensures that node, task and job info will remain unchanged in case of an error during SchedulerCache.Bind and SchedulerCache.Evict calls.
Before if error occurred during binding phase (e.g. "Selected node NotReady") task could get stuck in the Binding status indefinitely while the real pod would be in the Pending status.

- SchedulerCache.Evict and SchedulerCache.Bind will revert task status on NodeInfo.UpdateTask and NodeInfo.AddTask errors.
- Modified behavior of NodeInfo.AddTask. AddTask will now update task's node name upon successful addition, this is similar to how JobInfo.UpdateTaskStatus updates task's status.
- Handling unchecked error in JobInfo.UpdateTaskStatus.
- FATAL logging in NodeInfo.UpdateTask when impossible situation happens - failing to add a task after removal of a task from node info.

Might be related to #891
@mateuszlitwin mateuszlitwin force-pushed the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch from 063b55c to 455262b Nov 21, 2019
@k82cn

This comment has been minimized.

Copy link
Contributor

k82cn commented Nov 22, 2019

Ready for review.

Thanks very much! I'm going to check that :)

@k82cn

This comment has been minimized.

Copy link
Contributor

k82cn commented Nov 25, 2019

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 25, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, mateuszlitwin

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 4fc3170 into kubernetes-sigs:master Nov 25, 2019
3 of 4 checks passed
3 of 4 checks passed
tide Not mergeable. Retesting: pull-kube-batch-verify
Details
cla/linuxfoundation mateuszlitwin authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pull-kube-batch-verify Job succeeded.
Details
@mateuszlitwin mateuszlitwin deleted the mateuszlitwin:mlitwin-handle-errors-during-evict-and-bind branch Nov 25, 2019
@k82cn

This comment has been minimized.

Copy link
Contributor

k82cn commented Nov 26, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 26, 2019

@k82cn: GitHub didn't allow me to request PR reviews from the following users: sivanzcw, lminzhw.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @sivanzcw @lminzhw

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.