add new CDI hook to set CUDA memory limits [DO-NOT-MERGE] - #1995
add new CDI hook to set CUDA memory limits [DO-NOT-MERGE]#1995tariq1890 wants to merge 1 commit into
Conversation
6a201cf to
39d3e0f
Compare
tariq1890
changed the title
39d3e0f to
898335f
Compare
Coverage Report for CI Build 31854831036Coverage decreased (-0.4%) to 43.194%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
363243d to
4361243
Compare
tariq1890
changed the title
3265fc2 to
b0fcb92
Compare
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
b0fcb92 to
5ab636a
Compare
| @@ -0,0 +1,185 @@ | |||
| /** | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
nit: remove the year from the copyright header
| &cli.StringFlag{ | ||
| Name: "driver-root", | ||
| Usage: "Specify the driver root", | ||
| Destination: &cfg.driverRoot, |
There was a problem hiding this comment.
Question -- do we need to define a default value for this field, or validate that it is specified in validateFlags()?
There was a problem hiding this comment.
Good question. What would you suggest here?
| if !ok1 || !ok2 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Question -- does this mean that it is invalid to only specify one of these envvars (but not the other)?
There was a problem hiding this comment.
I am open to changing this. We can just limit it to 2 out of the 4 env vars here (maybe drop the plurals?) for the sake of simplicity here.
There was a problem hiding this comment.
My question was more so out of curiosity. Is it valid to only specify a memory limit and not a request?
There was a problem hiding this comment.
Yes, I believe it is. If unspecified, the value stays the same as it was
| @@ -0,0 +1,137 @@ | |||
| /** | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
nit: let's remove the year from the copyright header
| myCgroupPath := spec.Linux.CgroupsPath | ||
| parts := strings.Split(myCgroupPath, ":") | ||
| if len(parts) != 3 { | ||
| return "", fmt.Errorf("expected cgroupsPath to be of format \"slice:prefix:name\" for systemd cgroups, got %q instead", myCgroupPath) |
There was a problem hiding this comment.
Question -- does this only work when systemd is managing cgroups? What about cgroupfs?
There was a problem hiding this comment.
Yes, this PR only supports systemd as the cgroup manager in its current state. I think cgroupfs needs to be supported too.
There was a problem hiding this comment.
Let's add some unit tests for this file.
| deviceNodes, | ||
| ) | ||
|
|
||
| cudaMemoryLimitsHook, err := (*nvcdilib)(l.nvmllib).newCudaMemoryLimits(d) |
There was a problem hiding this comment.
Just curious, is the NVML API for setting memory limits only supported on full GPUs? What about MIG devices?
There was a problem hiding this comment.
I will need to find out
3 participants
This commit introduces a new CDI hook binary called
apply-cuda-memory-limits. The purpose of this CDI hook is to apply the desired memory limits to a GPU device.It reads the following envars from a container spec to fetch the input parameters for the CDI hook
Under the covers, the CDI hooks invokes the new
NVML SetMemoryLimits_v1method (introduced in CUDA 13.4) to apply the desired CUDA memory limit values to the cgroup of the GPU container in question