Supports the scheduling capabilities of SubGroup - #4721
Conversation
Summary of ChangesHello @ouyangshengjia, 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 significantly enhances the Volcano scheduler's capabilities by introducing a hierarchical scheduling model centered around 'PodBunches'. This new abstraction allows for the definition and enforcement of network topology constraints at a finer granularity than before, enabling more sophisticated and efficient resource allocation for complex workloads. The changes involve substantial modifications to the scheduler's internal data structures and allocation algorithms to accommodate the new PodBunch concept and ensure proper state management. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to support PodBunch scheduling capabilities, enhancing network topology awareness within the Volcano scheduler. The changes involve new data structures (allocateContext, JobWorksheet, PodBunchWorksheet, PodBunchInfo) to manage groups of pods with specific network topology constraints. The core allocation logic has been refactored to handle these PodBunches, including new functions for organizing, allocating, and selecting the best hypernodes for jobs and individual pod bunches. Additionally, the HyperNodeInfo structure has been extended to include parent-child relationships, improving the hierarchical representation of network topology. A new Recorder component is introduced to track allocation decisions. Test cases have been updated to reflect these new functionalities and configurations.
Overall, the changes are substantial and well-structured, addressing a complex scheduling requirement. However, there are a few areas that could benefit from clarification or further refinement to ensure robustness and full adherence to intended behavior, particularly concerning the handling of fit errors and the NeedContinueAllocating logic for jobs with BunchPolicy.
| // todo: job contains bunch policy does not supports the strategies below | ||
| if ji.ContainsBunchPolicy() { | ||
| return true | ||
| } |
There was a problem hiding this comment.
The comment // todo: job contains bunch policy does not supports the strategies below and the subsequent return true indicate a known limitation. For jobs with BunchPolicy, the detailed logic for checking task role minimums and other strategies is bypassed, causing NeedContinueAllocating to always return true.
This could lead to situations where scheduling continues even if specific pod bunch or task role constraints are not met, potentially resulting in inefficient allocations or unexpected behavior. This todo should be addressed to ensure that jobs with BunchPolicy are evaluated against their granular requirements.
There was a problem hiding this comment.
The function NeedContinueAllocating is a fast-fail judgement when allocating pods in a job. Not supporting this performance optimization feature in SubGroup scheduling scenarios does not cause an error. It would be better to support this feature through a seperate ISSUE/PR.
| var podBunchesAllocationScore float64 | ||
|
|
||
| // Clone jobWorksheet and rest job's fit err to make sure it's a clean cache when everytime filter a hyperNode and do not affect each other between hyperNodes. | ||
| job.ResetFitErr() |
There was a problem hiding this comment.
The job.ResetFitErr() call here clears all fit errors for the entire job before evaluating a new hypernode candidate. Subsequently, within the loop for each pod bunch, job.ResetPodBunchFitErr(podBunch.UID) is called (L419).
Since job.ResetFitErr() already clears the entire NodesFitErrors map, the job.ResetPodBunchFitErr call for each pod bunch becomes redundant in this context. While not a functional bug, it adds unnecessary operations. Consider if job.ResetFitErr() should be more granular or if job.ResetPodBunchFitErr is intended for a different scenario not covered by the current outer loop structure.
There was a problem hiding this comment.
ResetPodBunchFitErr clears all fit errors for the podBunch in the job before a new bunch-level hyperNode allocate attemption.
| // NewHyperNodesInfo initializes a new HyperNodesInfo instance. | ||
| func NewHyperNodesInfo(lister listerv1.NodeLister) *HyperNodesInfo { | ||
| ready := new(atomic.Bool) | ||
| ready.Store(true) |
There was a problem hiding this comment.
The ready atomic boolean is initialized to true here. However, the actual building of the hypernode cache (which populates hyperNodes, hyperNodesSetByTier, realNodesSet) happens later via UpdateHyperNode calls. If the scheduler attempts to use hypernode information (e.g., by checking HyperNodesReadyToSchedule) before the cache is fully populated and UpdateHyperNode has successfully completed its initial build, it might operate on incomplete or uninitialized data.
It would be safer to initialize ready to false and only set it to true after the first successful build of the hypernode cache, ensuring that the scheduler only proceeds when the data is reliably ready.
There was a problem hiding this comment.
the ready flag should be true if there is no hypernode in the cluster.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for scheduling capabilities of SubGroup (PodBunch) within PodGroup. It divides pods in a podgroup into multiple podbunches, each with its own network topology constraint, and schedules pods according to network topology constraints at both podgroup and podbunch levels.
Key Changes
- Introduced PodBunchInfo to represent groups of tasks within a job with their own topology constraints
- Added podBunch-level scheduling functions (PodBunchReady, PodBunchPipelined, PodBunchOrder, HyperNodeGradient)
- Refactored allocation logic to support hierarchical job/podBunch scheduling with network topology
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/scheduler/api/pod_bunch_info.go | New file defining PodBunchInfo structure and related logic |
| pkg/scheduler/api/job_info.go | Added podBunch management and network topology helper methods |
| pkg/scheduler/api/types.go | Updated function signatures to use PodBunchInfo instead of JobInfo |
| pkg/scheduler/framework/session.go | Added ClusterTopHyperNode, podBunch functions, and allocated hyperNode recovery |
| pkg/scheduler/actions/allocate/allocate.go | Complete refactoring to support hierarchical job/podBunch allocation |
| pkg/scheduler/plugins/gang/gang.go | Added podBunch order, ready, and pipelined functions |
| pkg/scheduler/plugins/priority/priority.go | Added podBunch order function |
| pkg/scheduler/plugins/network-topology-aware/network_topology_aware.go | Added hyperNode gradient and binpacking logic for podBunches |
| pkg/scheduler/cache/cache.go | Added updateJobInfo for updating allocated hyperNodes |
| pkg/scheduler/util/scheduler_helper.go | Updated to return both list and set of nodes by hyperNode |
Comments suppressed due to low confidence (3)
pkg/scheduler/plugins/network-topology-aware/network_topology_aware.go:1
- Corrected spelling of 'hypeNode' to 'hyperNode' in log message.
/*
pkg/scheduler/api/hyper_node_info.go:1
- When addChild returns an error at line 442, the function exits early but parentHn.Children.Insert(member) at line 461 is never executed. However, when the error is returned at line 456, childHn.Parent may have already been set at line 460, creating an inconsistent state. The Parent field should only be set after all validations pass.
/*
pkg/scheduler/framework/session.go:1
- The condition
nta.maxTier < tierwill cause ZeroScore to be returned when the tier exceeds maxTier. However, this doesn't prevent potential division by zero in line 501 if minTier equals maxTier and the first condition isn't caught. The logic should ensure minTier < maxTier before division.
/*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4c8906b to
ce325f0
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func getSubJobID(job JobID, policy string, matchValues []string) SubJobID { | ||
| id := strings.Join(matchValues, "-") | ||
| if len(id) > 128 { | ||
| hasher := fnv.New32a() | ||
| _, _ = hasher.Write([]byte(id)) | ||
| id = rand.SafeEncodeString(fmt.Sprint(hasher.Sum32())) // todo handle collision | ||
| } | ||
| return SubJobID(fmt.Sprintf("%s/%s-%s", job, policy, id)) | ||
| } |
There was a problem hiding this comment.
The TODO comment 'todo handle collision' indicates unfinished collision handling logic for hash-based SubJobID generation. This could lead to incorrect SubJob identification if collisions occur. Consider implementing proper collision detection or using a more robust ID generation strategy (e.g., UUID or cryptographic hash).
| if ji.ContainsSubJobPolicy() { | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
The TODO comment 'todo: job contains subJob policy does not supports the strategies below' indicates incomplete implementation for jobs with SubJob policies. The function returns true without checking failed roles when ContainsSubJobPolicy() is true, which may bypass important allocation logic. Consider implementing the full allocation strategy for jobs with SubJob policies.
| if ji.ContainsSubJobPolicy() { | |
| return true | |
| } | |
| // Allocation logic for jobs with SubJob policies is now handled below. |
There was a problem hiding this comment.
The function NeedContinueAllocating is a fast-fail judgement when allocating pods in a job. Not supporting this performance optimization feature in SubGroup scheduling scenarios does not cause an error. It would be better to support this feature through a seperate ISSUE/PR.
| if member.Type != topologyv1alpha1.MemberTypeHyperNode { | ||
| continue | ||
| } | ||
| if member.Selector.ExactMatch == nil { // todo support other selector method |
There was a problem hiding this comment.
The TODO comment indicates that only ExactMatch selectors are currently supported, while other selector methods (e.g., regex or label-based matching) are not implemented. This limitation should be documented or the other selector types should be implemented to avoid unexpected behavior.
| if member.Selector.ExactMatch == nil { // todo support other selector method | |
| if member.Selector.ExactMatch == nil { // Only ExactMatch selectors are currently supported; other selector methods (e.g., regex or label-based matching) are not implemented. | |
| fmt.Printf("Warning: Only ExactMatch selectors are supported in createSchedulerCache; selector for member %v will be ignored.\n", member) |
There was a problem hiding this comment.
I think retain todo tag is better
| if !ssn.HyperNodesReadyToSchedule && job.ContainsNetworkTopology() { | ||
| klog.V(4).Infof("Job <%s/%s> Queue <%s> skip allocate, reason: hyperNodes are not ready for scheduling", | ||
| job.Namespace, job.Name, job.Queue) | ||
| continue | ||
| } |
There was a problem hiding this comment.
[nitpick] This check duplicates similar logic that existed in the original code but was removed from the allocate loop. Consider centralizing the HyperNodesReadyToSchedule check to avoid scattered validation logic and potential inconsistencies.
There was a problem hiding this comment.
HyperNodesReadyToSchedule will not be changed within a session, so there is no potential inconsistencies.
f34e336 to
2ed3680
Compare
2ed3680 to
f7742ce
Compare
38fa842 to
d0c3616
Compare
|
The podBunch field should no longer be present after the update; it's recommended to refresh the PR description. |
| continue | ||
| } | ||
|
|
||
| // todo: Currently, jobs containing networkTopology do not support preemption. |
There was a problem hiding this comment.
Please clearly describe your next steps. It would be even better if you had issue tracking information. Please also include your GitHub username for easier communication with other contributors.
|
Should this PR be updated with a new design doc so that more contributors can understand the current framework? Our framework seems to be getting increasingly complex. |
Signed-off-by: ouyangshengjia <oysj2016@163.com>
Signed-off-by: ouyangshengjia <oysj2016@163.com>
52660e3 to
35d5ca7
Compare
…ent function Signed-off-by: ouyangshengjia <oysj2016@163.com>
Signed-off-by: ouyangshengjia <oysj2016@163.com>
Signed-off-by: ouyangshengjia <oysj2016@163.com>
Signed-off-by: ouyangshengjia <oysj2016@163.com>
Signed-off-by: ouyangshengjia <oysj2016@163.com>
Signed-off-by: ouyangshengjia <oysj2016@163.com>
419867f to
8d068b7
Compare
|
/lgtm |
Signed-off-by: ouyangshengjia <oysj2016@163.com>
8d068b7 to
9182c7d
Compare
|
[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 |
|
Does the subGroup support different priorityClass with PodGroup in future ? @ouyangshengjia @JesseStutler Consider such a scenario: In one job,
this scenario support job can use elastic resource or idle resource in k8s cluster. And those best-effort tasks can be preempting in any time without manual operation or job’s autoscaling. |
|
@yccharles What's your use case, please file a new issue? So others can see it and discuss there |
I think it's a good suggestion! @yccharles |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Partial fixes for #4188
Special notes for your reviewer:
@wangyang0616 @JesseStutler @hzxuzhonghu
Does this PR introduce a user-facing change?
Example YAML for vcJob after modification
Example YAML for podgroup after modification