Gang aware eviction design doc #4780
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @vzhou-p! It looks like this is your first PR to volcano-sh/volcano 🎉 |
Summary of ChangesHello @vzhou-p, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new design document outlining a significant enhancement to the Volcano scheduler: gang-aware preemption and reclaim. The proposed changes aim to resolve existing issues such as gang blindness, hardcoded topology logic, and topology ignorance in current scheduling actions. By introducing a plugin-driven pipeline and sophisticated victim selection strategies, the design seeks to improve resource utilization, ensure gang atomicity, and provide more intelligent and fair eviction mechanisms for distributed jobs. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
volcano-sh-bot
added
the
size/L
![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 is a well-written and comprehensive design document for gang-aware eviction. The proposed architecture with a 3-phase pipeline and new plugin extension points is a solid approach to address the current limitations. The detailed victim selection algorithm, using "Bundles" and an ROI-based scoring, is particularly well-thought-out. My review comments focus on clarifying some ambiguities and inconsistencies in the design to ensure it is fully specified and robust, particularly around function signatures and the definition of resource needs for preemption.
| # INPUT: | ||
| # - Preemptor: The job needing resources | ||
| # - Domain: The specific set of nodes we are clearing | ||
| # - Candidates: Jobs with tasks on these nodes | ||
| # - Session: Scheduler session (plugins) |
There was a problem hiding this comment.
The INPUT list for the SelectGangVictimsInDomain algorithm appears to be missing the NeededResources parameter. The algorithm logic later uses this value (e.g., Needed for Coverage score on line 203 and NeededResources in the selection loop on line 346), but it's not defined as an input. This is a critical piece of information for the victim selection process.
Additionally, the function call in section 3.3 (ssn.SelectGangVictimsInDomain(job, domain)) should be updated to pass this value once it's added to the signature.
| ROI_A = CalculateROI(A, Preemptor.Request, Cluster.Capacity) | ||
| ROI_B = CalculateROI(B, Preemptor.Request, Cluster.Capacity) |
There was a problem hiding this comment.
The document is inconsistent in how it refers to the resources required by the preemptor. Here, Preemptor.Request is used, while section 3.2.2 uses Preemptor.NeededResources.
More importantly, it's unclear how the resource requirement for a preempting job is defined. Is it the resource request of a single pending task, or the cumulative resources needed for the job to reach its minAvailable count? This definition is fundamental for the ROI calculation and the entire preemption logic. Please clarify what Preemptor.Request or NeededResources represents for a job.
| * **Logic (Primary Driver)**: The **First Plugin** that returns a non-empty result defines the Search Space. Subsequent plugins are ignored for domain generation. | ||
| * **Reasoning**: This "Winner-Takes-All" approach avoids complex intersection logic and ordering conflicts. The highest-priority plugin (e.g., `network-topology-aware`) drives the structural constraint. |
There was a problem hiding this comment.
The "Winner-Takes-All" approach for search space reduction is a reasonable simplification to avoid complex intersection logic. However, it prevents combining constraints from multiple plugins. For example, a user might want to apply both network topology constraints and other custom domain constraints defined in separate plugins.
Is this limitation intentional for the long-term design, or a simplification for the initial implementation? It would be helpful to clarify if there are future plans to support composing search spaces from multiple plugins.
| # 2. Call ReclaimableFn ONCE | ||
| # The plugin will approve tasks in order until constraints (like Queue Deserved) are met. | ||
| # It acts as a "Cut-Off" filter on our priority list. | ||
| AllowedTasksSet = Session.Reclaimable(Preemptor.AnyTask, OrderedCandidates).ToSet() |
There was a problem hiding this comment.
The term Preemptor.AnyTask in the call to Session.Reclaimable is a bit ambiguous. Could you please clarify what this represents? Is it a placeholder task created with the preemptor job's properties, or does it refer to a specific pending task from the job? Providing more detail on how this task is selected or constructed would improve the clarity of the design.
|
|
||
| // 4. Gang-Aware Victim Search (Domain-Wide) | ||
| // Returns optimal victims across the entire domain based on Bundle Strategy | ||
| victims, err := ssn.SelectGangVictimsInDomain(job, domain) | ||
| if err != nil { continue } | ||
|
|
||
| // 5. Simulate & Execute | ||
| if victims != nil && SimulateAndExecute(domain, victims, job) { | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
The indentation in this Go code block seems to be incorrect, which affects readability. Please consider fixing the indentation for consistency.
| // 4. Gang-Aware Victim Search (Domain-Wide) | |
| // Returns optimal victims across the entire domain based on Bundle Strategy | |
| victims, err := ssn.SelectGangVictimsInDomain(job, domain) | |
| if err != nil { continue } | |
| // 5. Simulate & Execute | |
| if victims != nil && SimulateAndExecute(domain, victims, job) { | |
| break | |
| } | |
| } | |
| for _, domain := range validDomains { | |
| // 4. Gang-Aware Victim Search (Domain-Wide) | |
| // Returns optimal victims across the entire domain based on Bundle Strategy | |
| victims, err := ssn.SelectGangVictimsInDomain(job, domain) | |
| if err != nil { | |
| continue | |
| } | |
| // 5. Simulate & Execute | |
| if victims != nil && SimulateAndExecute(domain, victims, job) { | |
| break | |
| } | |
| } |
2 participants
What type of PR is this?
Propose a design for gang-aware preempt and reclaim.
This design is for feature #4607
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?