feat: support module-pair allocation for Ascend 910C devices in SuperPod environments #1610
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ashergaga The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary of ChangesHello @ashergaga, 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 crucial enhancements to the HAMi scheduler, specifically targeting Ascend 910C devices in Huawei SuperPod environments. The changes ensure that NPU allocations adhere to the physical hardware topology, where devices are provisioned in module pairs. This prevents invalid resource requests, provides clearer feedback to users, and optimizes resource utilization by ensuring that only complete modules are allocated. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 support for module-pair allocation for Ascend 910C devices, which is a valuable enhancement. The logic correctly rounds up requests for a single NPU to two and rejects other odd-numbered requests. A new allocation strategy, computeBestCombination910C, is added to enforce module-pair selection. While the overall approach is sound, I've identified a critical issue where a partial device allocation could be incorrectly reported as successful. Additionally, there's a bug in the error handling path that would prevent scheduling failure events from being generated correctly for invalid request numbers. I've provided suggestions to fix these issues and improve code clarity and maintainability.
| if strings.Contains(reason, common.CardReqNumInvalid) { | ||
| failedNodes[nodeID] = "node device request must be 1 or even" | ||
| } else { | ||
| failedNodes[nodeID] = common.NodeUnfitPod | ||
| } | ||
| for reasonType := range common.ParseReason(reason) { | ||
| failureReason[reasonType] = append(failureReason[reasonType], nodeID) | ||
| } |
There was a problem hiding this comment.
This logic has a critical flaw. When reason contains common.CardReqNumInvalid, common.ParseReason(reason) will fail to parse the string, resulting in an empty map. Consequently, failureReason is not populated, and no scheduling failure event will be recorded for the pod. This makes debugging scheduling failures difficult.
You should handle the CardReqNumInvalid case separately to ensure the failure reason is correctly recorded, and only call ParseReason for reasons that are in the expected format.
| if strings.Contains(reason, common.CardReqNumInvalid) { | |
| failedNodes[nodeID] = "node device request must be 1 or even" | |
| } else { | |
| failedNodes[nodeID] = common.NodeUnfitPod | |
| } | |
| for reasonType := range common.ParseReason(reason) { | |
| failureReason[reasonType] = append(failureReason[reasonType], nodeID) | |
| } | |
| if strings.Contains(reason, common.CardReqNumInvalid) { | |
| failedNodes[nodeID] = "node device request must be 1 or even" | |
| failureReason[common.CardReqNumInvalid] = append(failureReason[common.CardReqNumInvalid], nodeID) | |
| } else { | |
| failedNodes[nodeID] = common.NodeUnfitPod | |
| for reasonType := range common.ParseReason(reason) { | |
| failureReason[reasonType] = append(failureReason[reasonType], nodeID) | |
| } | |
| } | |
| k.Nums = 2 | ||
| } else if k.Nums%2 != 0 { | ||
| // Reject any other odd-numbered request (e.g., 3, 5, 7...) | ||
| errMsg := fmt.Sprintf("CardReqNumInvalid: Ascend910C device request must be 1 or even number, got %d", k.Nums) |
There was a problem hiding this comment.
For better maintainability and to avoid hardcoding strings, it's recommended to use the common.CardReqNumInvalid constant when constructing the error message. This ensures consistency if the constant ever changes.
| errMsg := fmt.Sprintf("CardReqNumInvalid: Ascend910C device request must be 1 or even number, got %d", k.Nums) | |
| errMsg := fmt.Sprintf("%s: Ascend910C device request must be 1 or even number, got %d", common.CardReqNumInvalid, k.Nums) |
| return false, nil, errMsg | ||
| } | ||
| } | ||
| originReq := k.Nums |
There was a problem hiding this comment.
The variable originReq is now initialized after k.Nums has been potentially modified for Ascend 910C devices. This makes the name originReq misleading as it no longer holds the original request value but the adjusted one. For better code clarity and maintainability, consider renaming it to something like adjustedReqNum.
| // Group NPU indices by ther module and Sort | ||
| cardTopology := make(map[int][]int) | ||
| for _, idx := range npuIndices { | ||
| cardId := idx / MaxCardNPUNum | ||
| cardTopology[cardId] = append(cardTopology[cardId], idx) | ||
| } | ||
|
|
||
| // Convert the card topology map into a slice for sorting. | ||
| cardTopSlice := make([][]int, 0, len(cardTopology)) | ||
| for _, card := range cardTopology { | ||
| cardTopSlice = append(cardTopSlice, card) | ||
| } | ||
|
|
||
| // Sort cards by the number of available NPUs in ascending order. | ||
| sort.Slice(cardTopSlice, func(i, j int) bool { | ||
| return len(cardTopSlice[i]) < len(cardTopSlice[j]) | ||
| }) | ||
|
|
||
| // Select NPUs card by card, preferring full cards. | ||
| var selectedIndices []int | ||
| taskNPUNum := reqNum | ||
|
|
||
| for _, card := range cardTopSlice { | ||
| if taskNPUNum <= 0 { | ||
| break | ||
| } | ||
|
|
||
| // Only consider cards that have both NPUs available (full card). | ||
| if len(card) == MaxCardNPUNum { | ||
| selectedIndices = append(selectedIndices, card...) | ||
| taskNPUNum -= MaxCardNPUNum | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a minor typo in the comment on line 560 ("ther" should be "their").
More importantly, the logic for selecting full cards can be simplified for better readability and efficiency. Instead of converting the map to a slice and then sorting it, you can directly filter for cards that are full (i.e., have MaxCardNPUNum available NPUs) and then iterate over this filtered list. This makes the intent of the code more explicit.
// Group NPU indices by their module and Sort
cardTopology := make(map[int][]int)
for _, idx := range npuIndices {
cardId := idx / MaxCardNPUNum
cardTopology[cardId] = append(cardTopology[cardId], idx)
}
// Filter for cards that have a full module of NPUs available.
var fullCards [][]int
for _, cardIndices := range cardTopology {
if len(cardIndices) == MaxCardNPUNum {
fullCards = append(fullCards, cardIndices)
}
}
// Select NPUs from full cards until the request is met.
var selectedIndices []int
taskNPUNum := reqNum
for _, card := range fullCards {
if taskNPUNum <= 0 {
break
}
selectedIndices = append(selectedIndices, card...)
taskNPUNum -= MaxCardNPUNum
}
|
please sign-off your commit |
| return result | ||
| } | ||
|
|
||
| func (npudev *Devices) computeBestCombination910C(nodeInfo *device.NodeInfo, reqNum int, containerDevices device.ContainerDevices) device.ContainerDevices { |
There was a problem hiding this comment.
if set dev.CustomInfo["NetworkID"] = dev.Idx / 2, can the current computeBestCombination function meet your allocation requirements?
There was a problem hiding this comment.
Theoretically it's feasible, but I'm not sure if there might be some edge cases—like a module having only one gpu available—where the original computeBestCombination method might not meet the requirements.
There was a problem hiding this comment.
what's problems will occur if (id0, id2) are allocated with Ascend910C?
There was a problem hiding this comment.
The Ascend driver will hide both NPUs from npu-smi info since each group isn’t fully allocated, like this error in this image. It’s even possible that 4 NPUs are allocated, but npu-smi info only shows the 2 that belong to a fully allocated module.
@ashergaga Since NPUs in one physical module can not be allocated separately, why not to register as one unit in ascend-device-plugin ? |
|
|
||
| func (npu *Devices) Fit(devices []*device.DeviceUsage, request device.ContainerDeviceRequest, pod *corev1.Pod, nodeInfo *device.NodeInfo, allocated *device.PodDevices) (bool, map[string]device.ContainerDevices, string) { | ||
| k := request | ||
| if k.Type == Ascend910CType { |
There was a problem hiding this comment.
@ashergaga Can we move the check and modification to MutateAdmission ?
3 participants
This PR enhances the HAMi scheduler to properly handle Ascend 910C devices in Huawei SuperPod clusters by enforcing module-pair allocation: