BUGFIX: modifier: respect GPU volume-mount device requests #1130
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR privatizes the legacy VisibleDevicesFromEnvVar method and ensures all modifiers use the public VisibleDevices API to query requested devices.
- Swapped calls from
VisibleDevicesFromEnvVar()toVisibleDevices()in each modifier - Renamed and privatized
VisibleDevicesFromEnvVartovisibleDevicesFromEnvVar, and introduced a newvisibleEnvVarshelper - Updated the unit test to exercise the private
visibleDevicesFromEnvVarmethod
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/modifier/graphics.go | Replaced VisibleDevicesFromEnvVar with VisibleDevices |
| internal/modifier/gated.go | Replaced VisibleDevicesFromEnvVar with VisibleDevices |
| internal/modifier/csv.go | Replaced VisibleDevicesFromEnvVar with VisibleDevices |
| internal/modifier/cdi.go | Replaced VisibleDevicesFromEnvVar with VisibleDevices |
| internal/config/image/cuda_image_test.go | Switched test to call visibleDevicesFromEnvVar |
| internal/config/image/cuda_image.go | Privatized VisibleDevicesFromEnvVar, added visibleEnvVars, updated VisibleDevices |
Comments suppressed due to low confidence (2)
internal/config/image/cuda_image.go:233
- The new helper
visibleEnvVarsisn’t directly covered by existing tests. Consider adding unit tests to verify its behavior across different environment variable combinations.
func (i CUDA) visibleEnvVars() []string {
internal/config/image/cuda_image_test.go:432
- This test now calls an unexported method; ensure the test file is in the same package (not
*_test) so it can accessvisibleDevicesFromEnvVar, or consider testing via the publicVisibleDevicesmethod instead.
devices := image.visibleDevicesFromEnvVar()
Pull Request Test Coverage Report for Build 15468835763Details
💛 - Coveralls |
elezar
reviewed
Jun 5, 2025
| @@ -95,7 +95,7 @@ func getDevicesFromSpec(logger logger.Interface, ociSpec oci.Spec, cfg *config.C | |||
|
|
|||
| var devices []string | |||
| seen := make(map[string]bool) | |||
| for _, name := range container.VisibleDevicesFromEnvVar() { | |||
| for _, name := range container.VisibleDevices() { | |||
There was a problem hiding this comment.
I may have mentioned this in #1110, but we already process the mount devices from above, and although this MAY have the desired effect we may want to revisit how we handle CDI device requests and how we distinguish between these are "regular" requests.
My suggestion would be to:
- Keep
VisibleDevicesFromEnvVarpublic for now. - Update the other usages as per this PR.
- Handle CDI devices in a follow up.
|
Please update the commit messages / PR heading and description to better indicate intent. We are making this change to FIX A BUG. That is not clear from the commit messages and as such this will be missed in our release notes. |
|
Please also add unit tests to capture the new behaviour in the affected code. (ideally first failing with the existing implementation). This PR is not a blocker for the v1.18.0-rc.1 release. |
ArangoGutierrez
changed the title
The gated modifiers used to add support for GDS, Mofed, and CUDA Forward Comatibility only check the NVIDIA_VISIBLE_DEVICES envvar to determine whether GPUs are requested and modifications should be made. This means that use cases where volume mounts are used to request devices (e.g. when using the GPU Device Plugin) are not supported. This patch takes visibleDevicesFromEnvVar private, making VisibleDevices the only exported method to query valid devices. And edits the gated modifiers to use this func, ensuring device requests via mounts are also taken into acount. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
This was referenced Jun 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The gated modifiers used to add support for GDS, Mofed, and CUDA Forward Comatibility only check the NVIDIA_VISIBLE_DEVICES envvar to determine whether GPUs are requested and modifications should be made. This means that use cases where volume mounts are used to request devices (e.g. when using the GPU Device Plugin) are not supported.
This patch takes visibleDevicesFromEnvVar private, making VisibleDevices the only exported method to query valid devices.
And edits the gated modifiers to use this func, ensuring device requests
via mounts are also taken into acount.
Fixes: #1049