[YUNIKORN-2629] Adding a node can result in a deadlock

[pbacsko]

What is this PR for?

Fix deadlock problem by modifying locking in cache.Context.

Lock/unlock calls were removed where it doesn't seem to be necessary. In most cases, the state of the context is not modified at all. Only the scheduler cache is affected which has its own lock.

Testing done:

• make test multiple times
• BenchmarkSchedulingThroughPut with deadlock detector
• BenchmarkSchedulingThroughPut with race detector

What type of PR is it?

• - Bug Fix
• - Improvement
• - Feature
• - Documentation
• - Hot Fix
• - Refactoring

Todos

• - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2629

How should this be tested?

Screenshots (if appropriate)

Questions:

• - The licenses files need update.
• - There is breaking changes for older versions.
• - It needs documentation.

[chia7712]

@pbacsko thanks for this patch

[chenyulin0719]

Based on my understanding.
After this PR. One of the results is that the informer event handlers are no longer grab context lock when updating nodes or updating Pods. (NodeInformer/PodInformer go routine)

I'm wondering if there have any racing when informer events(Pods/Nodes) comes during below go routine running:

1. (main goroutine) InitializeState() -> I'm not quite confident.
2. (shim scheduler goroutine) schedule() -> Should be safe.
3. (dispatcher goroutine) -> Should be safe.

For the InitializeState(), I'm not quite confident to answer below questions:
Q: What will happen if NodeInformer update NodeA when InitializeState() is registring NodeA?
Q: What will happen if PodInformer update PodA when InitializeState() is registring PodA?

Is it still safe?

[wilfred-s]

1. (main goroutine) InitializeState() -> I'm not quite confident.

That should not be a problem.

For the InitializeState(), I'm not quite confident to answer below questions: Q: What will happen if NodeInformer update NodeA when InitializeState() is registring NodeA? Q: What will happen if PodInformer update PodA when InitializeState() is registring PodA?

Is it still safe?

The informers are first synced without event handling on. That gives us the base list of pods and nodes. All these listed objects are then processed. The event handler are turned off until we have done all that work. The step 5 in the init turns them on. At that point we no longer have to worry about the init code. The cache lock will prevent the simultaneous changes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.07%. Comparing base (138d53a) to head (70c749f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #859      +/-   ##
==========================================
- Coverage   68.10%   68.07%   -0.03%     
==========================================
  Files          70       70              
  Lines        7634     7600      -34     
==========================================
- Hits         5199     5174      -25     
+ Misses       2218     2210       -8     
+ Partials      217      216       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[craigcondit]

+1 LGTM.

[wilfred-s]

LGTM +1
The horrible fix needed in (ctx *Context) RemoveApplication(appID string) will disappear completely when we fix YUNIKORN-2782 as the function is not used