Skip to content

fix(ascend): reject over-capacity memory requests instead of defaulting to whole card #2543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Neal006 wants to merge 3 commits into from
+102 −0
Closed

fix(ascend): reject over-capacity memory requests instead of defaulting to whole card #2543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6f2ee0e
fix(ascend): reject over-capacity memory requests instead of defaulti…
Neal006 Aug 10, 2026
9432a48
test(ascend): cover the int32 clamp and the limits path
Neal006 Aug 10, 2026
cc01a6f
test(ascend): cover the memoryFactor path for over-capacity requests
Neal006 Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pkg/device/ascend/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"flag"
"fmt"
"math"
"slices"
"sort"
"strconv"
Expand Down Expand Up @@ -339,6 +340,13 @@ func (dev *Devices) GenerateResourceRequests(ctr *corev1.Container) device.Conta
memnum = int(memnums)
} else {
m, _ := dev.trimMemory(memnums)

@mesutoezdil mesutoezdil Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this only fixes the scheduler side. pod still passes admission, then fails later at fit. is that fine, or should mutateadmission also fall back to requests, like #2532 asks?

@Neal006 Neal006 Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is deliberate. Failing at Fit is the correct outcome either way, since the pod cannot run on any card. What this PR removes is the silent success, where the pod scheduled and was accounted as 100% of a card.

MutateAdmission reading requests as well is the other half of #2532, but it changes behaviour for every requests-only Ascend pod, not just the over-capacity ones. That is a wider blast radius than this fix and it deserves its own review. I would rather keep this PR to the scheduler path and send the admission change separately.

Happy to fold it in here instead if a maintainer prefers one PR.

if m <= 0 {
// No template and not the whole card can serve this
// request. Carry the requested value through so Fit
// rejects it, rather than letting the zero fall
// through to the whole-card default below.
m = min(memnums, math.MaxInt32)

@mesutoezdil mesutoezdil Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no test covers this maxint32 clamp path. can u add a case with a request above int32 max?

@Neal006 Neal006 Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered in 9432a48. The case requests 2147483648 and expects Memreq to equal math.MaxInt32.

@mesutoezdil mesutoezdil Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test does not set memoryfactor above 1. does the clamp still force a reject after the factor scaling runs?

@Neal006 Neal006 Aug 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, added a case in cc01a6f.

The factor is applied at line 324, before trimMemory runs, so trimMemory and the clamp both see the already scaled value. A larger factor pushes the request further over capacity, never under it, so the reject holds.

The new case in Test_GenerateResourceRequestsFactor uses factor 1000 against the shared 128 MiB request. That scales to 128000, which exceeds the 32768 MemoryCapacity, and the test asserts Memreq stays 128000 with MemPercentagereq 0. Before this fix that same input produced Memreq 0 and MemPercentagereq 100, which Fit reads as the whole card.

}
memnum = int(m)
}
}
Expand Down
94 changes: 94 additions & 0 deletions pkg/device/ascend/device_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package ascend
import (
"errors"
"fmt"
"math"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -1211,6 +1212,60 @@ func Test_GenerateResourceRequests(t *testing.T) {
Coresreq: int32(0),
},
},
{
name: "resourcememoryname larger than MemoryCapacity is carried through, not defaulted to the whole card",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
"huawei.com/Ascend910A": resource.MustParse("1"),
"huawei.com/Ascend910A-memory": resource.MustParse("65536"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: "Ascend910A",
Memreq: int32(65536),
MemPercentagereq: int32(0),
Coresreq: int32(0),
},
},
{
name: "resourcememoryname larger than MemoryCapacity on the limits path is carried through too",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
"huawei.com/Ascend910A": resource.MustParse("1"),
"huawei.com/Ascend910A-memory": resource.MustParse("65536"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: "Ascend910A",
Memreq: int32(65536),
MemPercentagereq: int32(0),
Coresreq: int32(0),
},
},
{
name: "resourcememoryname above int32 max is clamped instead of wrapping to zero",
args: corev1.Container{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
"huawei.com/Ascend910A": resource.MustParse("1"),
"huawei.com/Ascend910A-memory": resource.MustParse("2147483648"),
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: "Ascend910A",
Memreq: int32(math.MaxInt32),
MemPercentagereq: int32(0),
Coresreq: int32(0),
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down Expand Up @@ -1391,6 +1446,45 @@ func Test_GenerateResourceRequestsFactor(t *testing.T) {
Coresreq: int32(0),
},
},
{
name: "factor scales the request past MemoryCapacity",
dev: Devices{
config: VNPUConfig{
CommonWord: "Ascend910A",
ResourceName: "huawei.com/Ascend910A",
ResourceMemoryName: "huawei.com/Ascend910A-memory",
MemoryAllocatable: int64(32768),
MemoryCapacity: int64(32768),
MemoryFactor: int32(1000),
Templates: []Template{
{
Name: "vir02",
Memory: int64(2184),
AICore: int32(2),
}, {
Name: "vir04",
Memory: int64(4369),
AICore: int32(4),
}, {
Name: "vir08",
Memory: int64(8738),
AICore: int32(8),
}, {
Name: "vir16",
Memory: int64(17476),
AICore: int32(16),
},
},
},
},
want: device.ContainerDeviceRequest{
Nums: int32(1),
Type: "Ascend910A",
Memreq: int32(128000),
MemPercentagereq: int32(0),
Coresreq: int32(0),
},
},
{
name: "factor 0",
dev: Devices{
Expand Down