[YUNIKORN-3004] Lock User Trackers and Group Trackers map during dele…

[manirajv06]

…te operations

What is this PR for?

Lock userTrackers and groupTrackers map during delete operations.

What type of PR is it?

• - Bug Fix

Todos

• - Task

What is the Jira issue?

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

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 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.02%. Comparing base (72a2553) to head (c8192fe).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/scheduler/ugm/manager.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1007      +/-   ##
==========================================
+ Coverage   82.01%   82.02%   +0.01%     
==========================================
  Files          97       97              
  Lines       15623    15627       +4     
==========================================
+ Hits        12813    12818       +5     
  Misses       2532     2532              
+ Partials      278      277       -1     

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

[pbacsko]

Please also take a look at resetGroupEarlierUsage() and resetGroupEarlierUsage(). No locks there either and those are called during config refresh I believe. If that's the case, then lock/unlock pair must be added there too.

[manirajv06]

Please also take a look at resetGroupEarlierUsage() and resetGroupEarlierUsage(). No locks there either and those are called during config refresh I believe. If that's the case, then lock/unlock pair must be added there too.

I did already checked. resetGroupEarlierUsage() is called from clearEarlierSetLimits which locks the object before calling the earlier.

[wilfred-s]

We should have a test case for the empty group tracker removal after decrease, requesting a follow up jira

Besides that fix itself is good, all access is now locked.

[ryankert01]

Interest in helping create this test!
jira link: https://issues.apache.org/jira/browse/YUNIKORN-3005

---

@wilfred-s I saw something interesting while trying to create the test: we don't have logic to decrease qt.maxRunningApps and qt.maxResources in decreaseTrackedResource() on L159 while we requires them to be zero to be deleted.
Therefore, in decreaseTrackedResource() on L183, it will not be deleted.
Maybe we should relax/add some logic with qt.maxRunningApps and qt.maxResources ?

[chenyulin0719]

@ryankert01 I don't think we need to add other test-only functions for the test case.

The maxRunningApps and maxResources in QueueTracker are created from queue configurations and will not be changed during their lifetime. You should be able to create QueueTracker with nil maxRunningApps/maxResources by configureing the corresponding queue config.

[ryankert01]

@chenyulin0719 Great insight! I completely think in the wrong direction (:
PR submitted!