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

[pbacsko]

What is this PR for?

Prevent deadlock in registerNodes() by releasing/re-acquiring the write lock.

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.

[codecov]

Codecov Report

Attention: Patch coverage is 58.33333% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 67.23%. Comparing base (5f80f49) to head (4853d08).
Report is 3 commits behind head on master.

Files	Patch %	Lines
pkg/cache/context.go	58.33%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
- Coverage   67.33%   67.23%   -0.11%     
==========================================
  Files          70       70              
  Lines        7598     7611      +13     
==========================================
+ Hits         5116     5117       +1     
- Misses       2271     2280       +9     
- Partials      211      214       +3     

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

[chenyulin0719]

Hi @pbacsko,

The temporary release context lock method makes sense to me.

However, I guess the root cause of the deadlock is other tests.
When another test called 'dispatcher.UnregisterAllEventHandlers()',

• yunikorn-k8shim/pkg/cache/scheduler_callback_test.go

  Lines 452 to 455 in 08a9b93

  	func testUpdateNode(t *testing.T, expectedEvent string, response *si.NodeResponse) {
  	callback, _ := initCallbackTest(t, false, false)
  	defer dispatcher.UnregisterAllEventHandlers()
  	defer dispatcher.Stop()

the EventHandler will be cleaned and the wait group will never meet the count.

• yunikorn-k8shim/pkg/cache/context.go

  Line 1574 in 08a9b93

  dispatcher.RegisterEventHandler(handlerID, dispatcher.EventTypeNode, func(event interface{}) {

• yunikorn-k8shim/pkg/cache/context.go

  Lines 1607 to 1608 in 08a9b93

  	// wait for all responses to accumulate
  	wg.Wait()

So the context lock is not released.

[pbacsko]

the EventHandler will be cleaned and the wait group will never meet the count.

This problem also occurs in real environments when adding a node. It's not just test code which fails.

BTW our idea is that this fix is good enough for 1.5.2 (it has been validated by Jacob Salway) and even 1.6.0. We can do a more thorough review of Context later in a separate JIRA.

[wilfred-s]

I am OK with the change as it is for 1.5.2. We need to have a proper look at the context lock for the 1.6.0 release and we should try to prevent this change from becoming the final solution.

[pbacsko]

I am OK with the change as it is for 1.5.2. We need to have a proper look at the context lock for the 1.6.0 release and we should try to prevent this change from becoming the final solution.

OK. Putting it back to draft. I'll commit it directly to branch-1.5.