Separate resource limits for each priority class #2601
Conversation
| @@ -282,10 +283,31 @@ func (q *AggregatedQueueServer) getJobs(ctx context.Context, req *api.StreamingL | |||
| }) | |||
| } | |||
|
|
|||
| // Map queue names to priority factor for all active queues, i.e., | |||
There was a problem hiding this comment.
This is just moved to earlier in the same function.
| // Nodes to be considered by the scheduler. | ||
| lastSeen := q.clock.Now() | ||
| nodes := make([]*schedulerobjects.Node, 0, len(req.Nodes)) | ||
| allocatedByQueueForCluster := make(map[string]schedulerobjects.QuantityByPriorityAndResourceType) | ||
| allocatedByQueueAndPriorityClassForCluster := make(map[string]schedulerobjects.QuantityByTAndResourceType[string], len(queues)) |
There was a problem hiding this comment.
We now track resources by priority class name instead of priority class priority.
| @@ -39,83 +39,85 @@ func V1ResourceListFromResourceList(rl ResourceList) v1.ResourceList { | |||
| return rv | |||
| } | |||
|
|
|||
| type QuantityByPriorityAndResourceType map[int32]ResourceList | |||
| type QuantityByTAndResourceType[T comparable] map[T]ResourceList | |||
There was a problem hiding this comment.
I've replaced QuantityByPriorityAndResourceType with the more generic QuantityByTAndResourceType[int32], which also allows us to use QuantityByTAndResourceType[string], to track resources by priority class name. Based on some micro benchmarks I created, the performance of QuantityByPriorityAndResourceType and QuantityByTAndResourceType[int32] is identical.
severinson
changed the title
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2601 +/- ##
==========================================
+ Coverage 58.56% 58.59% +0.02%
==========================================
Files 236 236
Lines 30380 30365 -15
==========================================
Hits 17791 17791
+ Misses 11221 11207 -14
+ Partials 1368 1367 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| priorityFactorByActiveQueue := make(map[string]float64, len(activeQueues)) |
There was a problem hiding this comment.
Is this actually used anywhere?
Currently, per-queue resource limits are based on resources allocated to jobs of a particular priority. This causes problems when several priority classes have the same priority, since all jobs with priority classes of equal priority count towards the same limit. In this PR, we change the limits to be based on resources allocated to jobs of a particular priority class. This allows us to apply separate limits to different priority classes even when they have the same priority.
If at some point we want to better support using several priority classes with different priorities, I suggest we introduce explicit cumulative limits in a separate PR, similar to those we've had previously but not defined as part of the priority classes (since each priority class may not have a unique priority).