Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Statement unevict method doesn't update nodeInfo.Releasing field as expected #907

Closed
yodarshafrir1 opened this issue Nov 17, 2019 · 4 comments

Comments

@yodarshafrir1
Copy link
Contributor

Is this a BUG REPORT or FEATURE REQUEST?:

kind bug

What happened:
unevict method in file: statement.go doesn't reset the field Releasing of the task's nodeInfo to the expected value (the value of Releasing which was changed by evict method will remain as if the task was released from the node).

This means that when a task is running on a node, evict will be called and then unevict will be called, the Releasing value of the current nodeInfo will be as if the task was evicted from the node.

This happens because unevict calls AddTask. In file: node_info.go AddTask there's a condition to check if the task exists on this node and if so we return an error and don't set the Releasing value to the right value.

Instead the method unevict in file: statement.go should call UpdateTask (this is the fix). In UpdateTask the Releasing value will be updated as if the task was never evicted from the node.

What you expected to happen:
The Releasing value of the node should be as if the task was never evicted from its node.

How to reproduce it (as minimally and precisely as possible):
Create a use case where preempt evicts a task but fails to pipeline a task, then statement.Discard will be called.
Print the value Releasing field of the nodeinfo and see that it has a wrong value.

@yodarshafrir1 yodarshafrir1 changed the title Statement Discard method doesn't update nodeInfo.Releasing field as expected Statement unevict method doesn't update nodeInfo.Releasing field as expected Nov 17, 2019
@k82cn
Copy link
Contributor

k82cn commented Nov 19, 2019

is there anyway to trigger this case by workload?

@yodarshafrir1
Copy link
Contributor Author

yodarshafrir1 commented Nov 19, 2019

This happened to me after changing the order of the actions, so preempt will run before allocate.

When preempt will run, we just need an attempt to evict a task and yet, not have enough resources to pipeline the preemptor. On this case stmt.Discard() is called.
When Allocate will run after this case, due to the following if:

// Allocate releasing resource to the task if any.
task.InitResreq.LessEqual(node.Releasing) {

A pending task that fits the node.Releasing will be pipelined to the node, even though that the running task was never really evcited.

By the way, reading the comments in the code suggests that it was intended to update the task and not add a task.

// Update task in node.
if node, found := s.ssn.Nodes[reclaimee.NodeName]; found {
    node.AddTask(reclaimee)
}

@k82cn
Copy link
Contributor

k82cn commented Nov 27, 2019

thanks very much for your contribution :)

@yodarshafrir1
Copy link
Contributor Author

Issue was solved in: #912

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants