Generate CDI specification including additional GIDs #630
Conversation
|
Note: This updates the default spec version to v0.7.0 since the DRM devices generally require additional GIDs. As such, this should probably be an opt-in (or at least an opt-out) feature. |
elezar
force-pushed
the
CNT-4739/additional-gids
branch
from
74441f4 to
285ea44
Compare
| @@ -169,6 +174,11 @@ func (m command) build() *cli.Command { | |||
| Usage: "Specify a pattern the CSV mount specifications.", | |||
| Destination: &opts.csv.ignorePatterns, | |||
| }, | |||
| &cli.BoolFlag{ | |||
| Name: "--allow-additional-gids", | |||
| Usage: "Allow the use of the additionalGIDs field for generated CDI specifications. Note this will generate a v0.7.0 CDI specification.", | |||
There was a problem hiding this comment.
Which versions of containerd / cri-o (and other runtimes for that matter) support this?
There was a problem hiding this comment.
I would have to check. Note that this is not enabled by default when generating the CDI spec and only for internal representations.
There was a problem hiding this comment.
- containerd
- (>= 1.7.16) - containerd/containerd@7a2f49f
- (>= 1.8.0, >= 2.0.0) - containerd/containerd@1b62224
- docker (>= 26.1.0) - moby/moby@745e235
- podman (>= 5.1.0) - containers/podman@a40cf31
- crio (>= 1.30.0) - cri-o/cri-o@fd9aa76
internal/config/features.go
Outdated
| MOFED *feature `toml:"mofed,omitempty"` | ||
| NVSWITCH *feature `toml:"nvswitch,omitempty"` | ||
| GDRCopy *feature `toml:"gdrcopy,omitempty"` | ||
| NoAdditionalGIDs *feature `toml:"no-additional-gids,omitempty"` |
There was a problem hiding this comment.
Why is this reversed (i.e. no rather than allow) from the flag passed in?
There was a problem hiding this comment.
Because this has different default behaviour. For the NVIDIA Contianer Runtime I want to generated an internal representation including the additionalGIDs container edits for device nodes. This allows users to opt out if there is an issue.
I suppose we could make the argument that if we backport this (which we may want to due to the Jetpack feature request) we should keep the existing behaviour and not make these modifications either.
There was a problem hiding this comment.
Ok, after having thought this through again, I have disabled this by default and switched the feature flag to --allow-additional-gids.
internal/edits/device.go
Outdated
| deviceNode, err := d.toSpec() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var additionalGIDs []uint32 | ||
| if allowAdditionalGIDs { | ||
| if requiredGID, _ := d.getRequiredGID(); requiredGID != 0 { |
There was a problem hiding this comment.
Can we log the error instead of swallowing it ? Or we could just have the function not return an error struct
There was a problem hiding this comment.
Updated to not return an error.
This change refactors the creation of contianer edits to make provision for injecting options that affect the generated CDI specifications. Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
CNT-4739/additional-gids
branch
from
285ea44 to
141772c
Compare
elezar
force-pushed
the
CNT-4739/additional-gids
branch
2 times, most recently
from
7ecd467 to
fab0c6d
Compare
This change adds support for injecting additional GIDs using the internal CDI representations. (Applicable for Tegra-based systems and /dev/dri devices) This is disabled by default, but can be opted in to by setting the features.allow-additional-gids feature flag. This can also be done by running sudo nvidia-ctk config --in-place --set features.allow-additional-gids Signed-off-by: Evan Lezar <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
elezar
force-pushed
the
CNT-4739/additional-gids
branch
from
fab0c6d to
5d55813
Compare
This change adds logic to include the
AdditionalGIDsrequired for a device node in a generated CDI specification.When a device node requires specific group membership for access, a non-root user running in a container (e.g. using the
-uDocker command line argument) may not have access to the device.in the v0.7.0 CDI specification, support was added for specifying additional GIDs in the CDI specification and these changes extract the required information from the device nodes on the host if available.
This is disabled by default.
It can be enabled for internal CDI representations such as those used for Tegra-based systems or
/dev/dridevices by setting:in the config.toml file.
This can also be opted in to on a per-container basis by setting
NVIDIA_ALLOW_ADDITIONAL_GIDS=enabled.The
nvidia-ctkcommand can be used to toggle this feature by running:It can be enabled when running
nvidia-ctk cdi generateby specifying the--allow-additional-gidscommand line argument: