[YUNIKORN-3410] Fix self-deadlock on ClusterContext lock during partition removal by hedger9487 · Pull Request #1145 · apache/yunikorn-core

hedger9487 · 2026-09-03T14:32:40Z

Description

Fixes a self-deadlock on the ClusterContext write lock during partition removal:

During a configuration reload that drops or renames a partition (updateSchedulerConfig), and during RM partition removal events (removePartitionsByRMID), the caller stops the partition manager while holding the ClusterContext write lock (cc.Lock()).
partitionManager.Stop() calls manager.remove() synchronously (since YUNIKORN-2233). In the previous code, manager.remove() ended with manager.cc.removePartition(name), which attempted to acquire the non-reentrant cc.Lock() again, resulting in an immediate self-deadlock that hung the scheduling loop, REST endpoints, and RM callbacks.

This PR:

Drops the manager.cc.removePartition() call from partitionManager.remove(), keeping remove() focused solely on cleaning up internal partition resources (queues, applications, nodes).
Removes the unused private removePartition() callback from ClusterContext.
In updateSchedulerConfig() and removePartitionsByRMID(), deletes removed partitions directly from cc.partitions during iteration under the already-held cc.Lock(), eliminating intermediate slice/map allocations (per maintainer feedback).
Wraps the pc.stateTime write in handlePartitionEvent() with pc.Lock() / pc.Unlock() to eliminate potential unprotected concurrent writes.
Adds TestContext_RemovePartition in context_test.go covering config reload partition removal, RM partition removal events, and active partition removal with running applications and nodes without duplicate fixtures.

Type of change

Jira issue

Jira ID : https://issues.apache.org/jira/browse/YUNIKORN-3410

I have created a Jira issue for this pull request.
The Jira ID is part of the title of this pull request.

AI Tooling

If an AI tool was used:

The PR includes the phrase "Generated by Antigravity", where Antigravity is the name of the AI tool used.
My use of AI contributions follows the ASF legal policy.

Check https://www.apache.org/legal/generative-tooling.html for details.

How has this been tested?

Deadlock reproduction and active application/node cleanup verified in TestContext_RemovePartition.
Full test suite run under -race (go test -race ./pkg/scheduler/...), 100% passed.
Local linter passed with 0 issues in modified files (golangci-lint run pkg/scheduler/...).
Full repository regression suite (go test ./...), 100% passed with zero regressions.

Questions:

The change needs documentation, a pull request for apache/yunikorn-site repository will be created.
There is breaking changes for older versions: jira is tagged with release-notes label.
The licenses files needs to be updated.

Screenshots or other details

Generated by hedger9487 with assistance from Antigravity.

codecov · 2026-09-03T14:49:32Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.98%. Comparing base (d13826c) to head (e741b76).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
+ Coverage   82.89%   82.98%   +0.09%     
==========================================
  Files         104      104              
  Lines       14467    14462       -5     
==========================================
+ Hits        11992    12001       +9     
+ Misses       2185     2176       -9     
+ Partials      290      285       -5

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

manirajv06 · 2026-09-07T08:19:57Z

 	}
+	cc.Lock()
+	defer cc.Unlock()
+	cc.partitions = make(map[string]*PartitionContext)


Is this required for unit tests?

Hi @manirajv06, thanks for pointing this out! To provide some context on why this was added:

In the original master code, ClusterContext.Stop() called part.partitionManager.Stop(), which called manager.remove(), which indirectly invoked manager.cc.removePartition(name) to delete each partition from cc.partitions.

When we removed the cc.removePartition() callback to fix the self-deadlock in updateSchedulerConfig and removePartitionsByRMID, ClusterContext.Stop() also lost that indirect cleanup side-effect. To prevent a behavioral regression where cc.partitions would remain populated after ClusterContext.Stop(), we explicitly cleared cc.partitions under cc.Lock(), and added the test to verify shutdown behavior was preserved.

However, if the community prefers ClusterContext.Stop() to only stop background services without resetting cc.partitions, I'm completely happy to drop it. What do you think?

I'm completely happy to drop it.

Yes, please

Dropped ClusterContext.Stop() changes and restored it back to original master.

manirajv06 · 2026-09-07T08:22:07Z

 	assert.Assert(t, checked, "Failed to find metric")
 }
+
+func TestUpdateSchedulerConfigRemovePartition(t *testing.T) {


Is it possible to wrap this, next and next to next and even last one into single test method using t.run() ?

Agree that these are the different situations needs to be tested but not each one requires a separate test method.

Done! Consolidated all 4 test scenarios into TestContext_RemovePartition using t.Run() subtests:

remove via updateSchedulerConfig

remove via removePartitionsByRMID

remove with active apps and nodes

stop clears partitions

All subtests pass cleanly under the race detector.

Nice. Can we avoid repeating the same code for each run?

Have only single run and execute all code and finally assert each case expected output. There are lot of references out there in the codebase. Please take a look.

t.run() {

}

Updated! Refactored TestContext_RemovePartition to use a single shared setup at the top, followed by t.Run() subtests for each case:

remove via updateSchedulerConfig: tests partition removal and active app/node teardown without deadlock.

remove via removePartitionsByRMID: tests remaining partition cleanup via RM event without deadlock.

manirajv06

looks good. Minor nits. Fix lint warnings as well.

hedger9487 · 2026-09-09T08:45:06Z

Hi @manirajv06,

I have updated the PR to address all review comments:

Reverted ClusterContext.Stop(): Removed the mutex lock and map re-initialization, restoring Stop() back to its original implementation.
Refactored TestContext_RemovePartition: Consolidated into a single shared cluster context setup with clean t.Run() subtests (remove via updateSchedulerConfig and remove via removePartitionsByRMID), testing active node and application cleanup without duplicate setup code or duplicate YAML fixtures.
Fixed Lint Warnings: Resolved all duplicate string definitions (goconst), and verified that golangci-lint run pkg/scheduler/... reports 0 issues in the modified files.

All unit tests pass cleanly under -race. Thanks again for the review!

adityadtu5

LGTM

manirajv06

Sorry for the delay. Review took more time than expected as it raised a question "Is Partition draining?" and so on..After looking at the old history of changes and further exploration, we had decided to raise #1174 to reinstate the partition state transitions which got missed somewhere along the way. Combining that PR with this should make partition transitions to look better. Once that PR goes in, please rebase and address the comments.

manirajv06 · 2026-10-06T08:03:34Z

 	if err == nil {
+		pc.Lock()
 		pc.stateTime = time.Now()
+		pc.Unlock()


Can we file a follow up for Queue as well?

Filed follow-up Jira YUNIKORN-3508 for Queue.

manirajv06 · 2026-10-06T08:04:19Z

+		})
+
+		result := <-c
+		assert.Assert(t, result.Succeeded)


You might need to assert the state after #1174 goes in

Done! Rebased onto master and added isDraining() / IsRunning() state assertions in both subtests.

…tion removal During a configuration reload that drops a partition, or during RM partition-removal events (removePartitionsByRMID), the caller stops the partition manager while holding the ClusterContext write lock. partitionManager.Stop() calls remove() synchronously, which was calling cc.removePartition(name). Because cc.removePartition() attempts to acquire the non-reentrant cc.Lock(), the infrastructure event goroutine deadlocked on itself, freezing the scheduling loop, health checks, and RM callbacks. Fixes: 1. Drop manager.cc.removePartition() from partitionManager.remove(). 2. Remove the unused private removePartition() callback from ClusterContext. 3. In updateSchedulerConfig() and removePartitionsByRMID(), delete removed partitions directly from cc.partitions during iteration under cc.Lock(). 4. Protect pc.stateTime write in handlePartitionEvent() with pc.Lock(). 5. Add unit test covering deadlock reproduction (config update and RM removal) and active app/node cleanup.

hedger9487 · 2026-10-08T16:05:48Z

Hi @manirajv06,

Thanks for reinstating the partition state transitions in #1174! I have updated the PR:

Rebased onto the latest master (incorporating [YUNIKORN-3483] Reinstate Partition state transitions #1174).
Added state assertions (isDraining() / IsRunning()) in TestContext_RemovePartition for both partition removal paths.
Filed follow-up Jira YUNIKORN-3508 to cover Queue.stateTime lock synchronization.

manirajv06 assigned hedger9487 Sep 4, 2026

manirajv06 requested review from manirajv06 and wilfred-s September 4, 2026 06:35

manirajv06 requested changes Sep 4, 2026

View reviewed changes

Comment thread pkg/scheduler/context.go Outdated

hedger9487 force-pushed the YUNIKORN-3410 branch from 416b70b to eff4d6b Compare September 4, 2026 15:29

hedger9487 requested a review from manirajv06 September 4, 2026 15:36

hedger9487 force-pushed the YUNIKORN-3410 branch 4 times, most recently from 2dac277 to 9b37668 Compare September 5, 2026 19:23

manirajv06 requested changes Sep 7, 2026

View reviewed changes

hedger9487 force-pushed the YUNIKORN-3410 branch from 9b37668 to 2f7a2cd Compare September 7, 2026 10:49

manirajv06 requested changes Sep 9, 2026

View reviewed changes

hedger9487 force-pushed the YUNIKORN-3410 branch from 2f7a2cd to 06f5646 Compare September 9, 2026 08:44

hedger9487 force-pushed the YUNIKORN-3410 branch from 06f5646 to ed5ef83 Compare September 9, 2026 09:06

hedger9487 requested a review from manirajv06 September 19, 2026 09:46

HuangTing-Yao requested review from HuangTing-Yao and chenyulin0719 September 19, 2026 13:16

hedger9487 force-pushed the YUNIKORN-3410 branch from ed5ef83 to e741b76 Compare September 29, 2026 19:03

adityadtu5 approved these changes Sep 30, 2026

View reviewed changes

rjgoyln mentioned this pull request Oct 1, 2026

[YUNIKORN-3483] Reinstate Partition state transitions #1174

Closed

4 of 15 tasks

manirajv06 reviewed Oct 6, 2026

View reviewed changes

hedger9487 force-pushed the YUNIKORN-3410 branch from e741b76 to 0889f41 Compare October 8, 2026 16:05

Conversation

hedger9487 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Jira issue

AI Tooling

How has this been tested?

Questions:

Screenshots or other details

Uh oh!

codecov Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manirajv06 Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manirajv06 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedger9487 commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityadtu5 left a comment

Choose a reason for hiding this comment

Uh oh!

manirajv06 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hedger9487 commented Oct 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hedger9487 commented Sep 3, 2026 •

edited

Loading

codecov Bot commented Sep 3, 2026 •

edited

Loading

manirajv06 Sep 7, 2026 •

edited

Loading

manirajv06 left a comment •

edited

Loading

hedger9487 commented Sep 9, 2026 •

edited

Loading

manirajv06 left a comment •

edited

Loading