Add EvictableFn Evolution for Gang-aware Eviction design - #5170
Conversation
Signed-off-by: Victor Zhou <vzhou@pinterest.com>
volcano-sh-bot
added
the
size/L
There was a problem hiding this comment.
Pull request overview
This PR adds a new design document that proposes an evolution of the EvictableFn plugin callback to support gang-aware eviction. The document is a follow-up to the existing gang-aware-eviction-design.md and addresses two key problems: the "representative task" problem (where a single task is used as proxy for an entire job) and the "implicit mode" problem (where the same callback signature is used for both legacy and gang-aware eviction without distinguishing between them).
Changes:
- New design document that proposes
UnifiedEvictableFnwithEvictionContextas the solution, providing an opt-in mechanism for plugins to support gang-aware eviction - Includes detailed pros/cons analysis and consideration of two alternative approaches
- Maintains backward compatibility while introducing a clear upgrade path
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design for UnifiedEvictableFn and EvictionContext to support gang-aware eviction. The review feedback suggests adding an Unknown value to the EvictionKind enum for safer initialization, defining the Task field's behavior in gang contexts to resolve the representative task issue, and clarifying the scope of PendingTasks to ensure consistency across plugin implementations.
| type EvictionKind int | ||
|
|
||
| const ( | ||
| GangEvictPreempt EvictionKind = iota |
There was a problem hiding this comment.
It is recommended to define an Unknown or Unspecified value as the first element of the EvictionKind enum. Since Go initializes integers to 0 by default, having a specific Unknown value helps detect uninitialized EvictionContext structs, which is a concern explicitly mentioned in the "Cons" section (line 75).
| GangEvictPreempt EvictionKind = iota | |
| EvictUnknown EvictionKind = iota | |
| GangEvictPreempt |
| type EvictionContext struct { | ||
| Kind EvictionKind | ||
| Job *JobInfo | ||
| Task *TaskInfo |
There was a problem hiding this comment.
To effectively resolve the "representative task" problem mentioned in the motivation, the design should explicitly define the contract for the Task field during gang-aware operations. Specifying that Task should be nil for GangEvictPreempt and GangEvictReclaim would ensure that plugins are forced to rely on the Job and PendingTasks fields for their logic, rather than falling back to a single representative task.
| Job *JobInfo | ||
| Task *TaskInfo | ||
|
|
||
| PendingTasks []*TaskInfo |
There was a problem hiding this comment.
It would be beneficial to clarify the expected content of PendingTasks. For gang-aware eviction, does this include all pending tasks of the job, or only the subset currently being considered for placement? Providing a clear definition will help maintain consistency across different plugin implementations.
There was a problem hiding this comment.
I think you can also give migration plan section, this section explains which plugins currently need to implement these new extension points, and if new plugins need to implement them in the future, what best practices guide their implementation. It also covers when existing evictableFn plugins should be deprecated, and the version plan for migrating to the new extension points.
JesseStutler
left a comment
JesseStutler
left a comment
There was a problem hiding this comment.
I prefer the new extension points since EvictionContext looks like more extensible :)
Signed-off-by: Victor Zhou <vzhou@pinterest.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JesseStutler The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
volcano-sh-bot
added
the
approved
What type of PR is this?
Add EvictableFn Evolution for Gang-aware Eviction design
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?