-
Notifications
You must be signed in to change notification settings - Fork 353
Add the ability to disable specific (or all) CDI hooks when generating a CDI specification #1077
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new CLI flag (--skip-hooks) to allow users to opt out of specific hooks when generating the CDI specification.
- Updated tests to include a skip-hook option.
- Modified the command initialization and spec generation logic to support disabling hooks based on the provided flag.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
cmd/nvidia-ctk/cdi/generate/generate_test.go | Added a test case to validate the skip-hook functionality and updated expected spec/options accordingly. |
cmd/nvidia-ctk/cdi/generate/generate.go | Introduced a new CLI flag and added logic to disable hooks based on user input. |
Comments suppressed due to low confidence (1)
cmd/nvidia-ctk/cdi/generate/generate_test.go:40
- [nitpick] The variable name 'skipdHook' may be a typo and could be renamed to 'skipHook' for consistency and clarity.
skipdHook := cli.NewStringSlice("enable-cuda-compat")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for a new flag (--skip-hooks) to the nvidia-ctk cdi generate command so that users can opt out of specific hooks when creating the CDI spec.
- In generate.go, a new skipHook option and corresponding CLI flag are introduced and processed when creating CDI library options.
- In generate_test.go, a new test case verifies the behavior when a non-default hook is skipped.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
cmd/nvidia-ctk/cdi/generate/generate_test.go | Added test case for --skip-hooks flag and its effect. |
cmd/nvidia-ctk/cdi/generate/generate.go | Introduced skipHook flag and integrated it into initialization options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new flag --skip-hooks to the cdi generate command to allow users to disable specific hooks while generating the CDI specification.
- Introduces a new CLI flag in generate.go to capture the hooks to skip
- Updates the options struct and test cases in generate_test.go to include and validate the new skipHook functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
cmd/nvidia-ctk/cdi/generate/generate_test.go | Adds a test case for skipHook using a new CLI string slice |
cmd/nvidia-ctk/cdi/generate/generate.go | Registers the new --skip-hook flag and passes the provided hooks to disable options during CDI spec generation |
Comments suppressed due to low confidence (1)
cmd/nvidia-ctk/cdi/generate/generate_test.go:40
- [nitpick] Consider adding a test case to verify that when a hook name is provided via skipHook, the generated CDI specification omits or disables that hook, ensuring the new flag's functionality is fully verified.
skipHook := cli.NewStringSlice("enable-cuda-compat")
@@ -176,6 +177,12 @@ func (m command) build() *cli.Command { | |||
Usage: "Specify a pattern the CSV mount specifications.", | |||
Destination: &opts.csv.ignorePatterns, | |||
}, | |||
&cli.StringSliceFlag{ | |||
Name: "skip-hook", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about disable-hook
(we could even add an alias as disable-hooks
) instead? Thinking ahead, it may be useful to allow users to do something like:
nvidia-ctk cdi generate \
--disable-hook=update-ldcache \
--disable-hook=enable-cuda-compat \
--mode=nvml
Even further forward, more conservative users may want to consider:
nvidia-ctk cdi generate --disable-hooks=all
although that is out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for a new flag that allows users to disable specific hooks during the CDI spec generation. Key changes include:
- Adding conditional logic to check for supported hooks in the NVML driver library.
- Introducing the new CLI flag (and corresponding option) to allow disabling hooks.
- Updating tests to reflect the new --disable-hook(s) functionality.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
pkg/nvcdi/driver-nvml.go | Wraps hook-based discoverer creation in conditionals based on hook support. |
cmd/nvidia-ctk/cdi/generate/generate_test.go | Adds test cases and sample disable-hook values in accordance with the new functionality. |
cmd/nvidia-ctk/cdi/generate/generate.go | Introduces a new flag and integrates hook disabling into the initialization options. |
Comments suppressed due to low confidence (1)
cmd/nvidia-ctk/cdi/generate/generate.go:181
- [nitpick] Consider renaming the flag from "disable-hook" to "disable-hooks" to match the PR title and description for consistency.
Name: "disable-hook",
64375df
to
b32d8a9
Compare
pkg/nvcdi/api.go
Outdated
) | ||
|
||
// NewHookName takes a string and returns a []HookName, empty if the HookName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the right place to implement this. we have a pkg/nvcdi/hooks.go
file where this can go instead.
Also, in terms of the implementation, does adding special handling to the disabledHooks
type for the all
hook not make this cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
if len(opts.disableHooks.Value()) > 0 { | ||
for _, hook := range opts.disableHooks.Value() { | ||
for _, hookName := range nvcdi.NewHookName(hook) { | ||
initOpts = append(initOpts, nvcdi.WithDisabledHook(hookName)) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The len
check is not required. One can always iterate over an empty slice.
if len(opts.disableHooks.Value()) > 0 { | |
for _, hook := range opts.disableHooks.Value() { | |
for _, hookName := range nvcdi.NewHookName(hook) { | |
initOpts = append(initOpts, nvcdi.WithDisabledHook(hookName)) | |
} | |
} | |
} | |
for _, hook := range opts.disableHooks.Value() { | |
initOpts = append(initOpts, nvcdi.WithDisabledHook(hook)) | |
} |
(note that I have updated WithDisabledHook
to:
// WithDisabledHook allows specific hooks to the disabled.
// This option can be specified multiple times for each hook.
func WithDisabledHook[T string | HookName](hook T) Option {
return func(o *nvcdilib) {
if o.disabledHooks == nil {
o.disabledHooks = make(map[HookName]bool)
}
o.disabledHooks[HookName(hook)] = true
}
}
to allow it to accept both string
and HookName
arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion taken
@@ -57,6 +57,7 @@ type options struct { | |||
|
|||
configSearchPaths cli.StringSlice | |||
librarySearchPaths cli.StringSlice | |||
disableHooks cli.StringSlice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disableHooks cli.StringSlice | |
disabledHooks cli.StringSlice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/nvcdi/driver-nvml.go
Outdated
l.nvidiaCDIHookPath, | ||
) | ||
discoverers = append(discoverers, driverDotSoSymlinksDiscoverer) | ||
if l.HookIsSupported(HookCreateSymlinks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this hook specifically (and the update-ldcache hook too), there are many places in the code where we are injecting this hook. As such this is not going to be sufficient to prevent their injection. This was why I suggested that handling these hooks is out of scope for this PR.
internal/discover/hooks.go
Outdated
} | ||
|
||
func NewHookCreator(nvidiaCDIHookPath string) HookCreator { | ||
func NewHookCreator(nvidiaCDIHookPath string, disabledHooks DisabledHooks) HookCreator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using:
func NewHookCreator(nvidiaCDIHookPath string, disabledHooks DisabledHooks) HookCreator { | |
func NewHookCreator[T string|HookName](nvidiaCDIHookPath string, disabledHooks ...T) HookCreator { |
The fact that we're using a map should be internal to this pacakge and not exposed through the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead I used the options route, let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I have added a commit on top that also sets the hook path as an option for consistency.
internal/discover/hooks.go
Outdated
func (d DisabledHooks) Set(value HookName) { | ||
if value == "all" { | ||
for _, hook := range AllHooks { | ||
d[hook] = true | ||
} | ||
return | ||
} | ||
|
||
d[value] = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, we can add an isDisabled(HookName)
function (something like we had before in IsHookSupported
that has been removed). We can impliment it as follows:
// HookIsSupported checks whether a hook of the specified name is supported.
// Hooks must be explicitly disabled, meaning that if no disabled hooks are
// all hooks are supported.
func (d disabledHooks) isDisabled(h HookName) bool {
if d["all"] {
return true
}
return d[h]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func removed, it was not needed by moving to a options implementation
9aa88ea
to
f72732e
Compare
@@ -270,7 +270,7 @@ func (m command) generateSpec(opts *options) (spec.Interface, error) { | |||
deviceNamers = append(deviceNamers, deviceNamer) | |||
} | |||
|
|||
initOpts := []nvcdi.Option{ | |||
cdiOptions := []nvcdi.Option{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: initOptions
was too generic.
@@ -127,7 +124,7 @@ containerEdits: | |||
vendor: "example.com", | |||
class: "device", | |||
driverRoot: driverRoot, | |||
disabledHooks: *disableHook1, | |||
disabledHooks: valueOf(cli.NewStringSlice("enable-cuda-compat")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of arbitrary names for the variables made the intent of the tests more difficult to understand. A valueOf
helper allows us to return the value of a string slice and list the disabled hooks in the tests.
// A HookName represents a supported CDI hooks. | ||
type HookName string | ||
|
||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the following changes:
- moved the constants earlier in the file as this is common practice in golang
- renamed the types
*Hook
instead ofHook*
. This allows the code at the callsite to read more naturally. It also seems more idiomatic (https://google.github.io/styleguide/go/decisions.html#constant-names). - added an
AllHooks
constant. - added a deprecated
ChmodHook
constant.
// cache inside the directory path to be mounted into a container. | ||
HookUpdateLDCache = HookName("update-ldcache") | ||
) | ||
type cdiHookCreator struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the type private and renamed it for clarity.
internal/discover/hooks.go
Outdated
type HookName string | ||
|
||
// disabledHooks allows individual hooks to be disabled. | ||
type disabledHooks map[HookName]bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type was not used since we were not attaching methods to it.
internal/discover/hooks.go
Outdated
HookEnableCudaCompat, | ||
HookCreateSymlinks, | ||
HookUpdateLDCache, | ||
fixedArgs []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking the path on every Create
invocation, we check it once at construction.
} | ||
|
||
type Option func(*CDIHook) | ||
// An allDisabledHookCreator is a HookCreator that does not create any hooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking disabledHooks
on every call to Create
, we instantiate a type that creates no hooks. (This is the null object pattern).
} | ||
|
||
// isDisabled checks if the specified hook name is disabled. | ||
func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the isDisabled
check to a method and made this dependent on the args
as well. This allows for the create-symlinks
and chmod
hooks to be handled more cleanly.
return append(c.fixedArgs, string(name)) | ||
} | ||
|
||
func (c cdiHookCreator) transformArgs(name HookName, args ...string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a function to handle the per-hook transforms.
} | ||
|
||
// isDisabled checks if the specified hook name is disabled. | ||
func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the isDisabled
check to a method and made this dependent on the args
as well. This allows for the create-symlinks
and chmod
hooks to be handled more cleanly.
@@ -113,7 +113,7 @@ func TestWithWithDriverDotSoSymlinks(t *testing.T) { | |||
expectedHooks: []Hook{ | |||
{ | |||
Lifecycle: "createContainer", | |||
Path: "/path/to/nvidia-cdi-hook", | |||
Path: "/usr/bin/nvidia-cdi-hook", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a specific path here wasn't adding value. Changed the tests to check the default.
c6428c1
to
caf855b
Compare
Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>
This change adds the ability to disabled specific (or all) CDI hooks to both the nvidia-ctk cdi generate command and the nvcdi API. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]> Signed-off-by: Evan Lezar <[email protected]>
This change adds the ability to disable specific (or all) CDI hooks when generating a CDI specification. This is supported in the
nvidia-ctk cdi generate
command through a--disable-hook
command line argument (that can be repeated):Specifying:
(or
--disable-hook=all
) will generate a CDI specification with NO NVIDIA-specific CDI hooks.Note that when using such as spec, the resultant container may not function in the same way as a regular container due to the ldcache not being updated, for example.
When using the
nvcdi
API, the functional optionWithDisabledHook("enable-cuda-compat")
orWithDisabledHook("all")
can be used. (Constants were added for supported hooks).Fixes: #1074