Add --disable-hook flag to cdi generate command #1077

ArangoGutierrez · 2025-05-12T16:53:55Z

When running the nvidia-ctk cdi generate command, a user should be able to opt out of specific hooks. We propose to add a flag --disable-hooks that will take a comma-separated list of hooks that will be skipped when creating the CDI spec.

Fixes: #1074

Copilot

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")

Copilot

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.

cmd/nvidia-ctk/cdi/generate/generate.go

Copilot

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")

cmd/nvidia-ctk/cdi/generate/generate.go

elezar · 2025-05-13T13:58:21Z

cmd/nvidia-ctk/cdi/generate/generate.go

+	if len(opts.skipHook.Value()) > 0 {
+		for _, hook := range opts.skipHook.Value() {
+			initOpts = append(initOpts, nvcdi.WithDisabledHook(nvcdi.HookName(hook)))
+		}
+	}


Did we want to check for validity of the names, or do we want to add that as a follow-up?

let's hold this PR until:

Ensure that libcuda.so is in the ldcache #947

Disable device node creation in CDI mode #927
get in, then I will think on how to add a name check.

let's hold this PR until:

Should this PR not just work automatically with those PRs -- assuming those hooks can be opted out of using the same option?

It now allows for disabling all hooks

elezar · 2025-05-13T20:37:59Z

cmd/nvidia-ctk/cdi/generate/generate.go

@@ -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",


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.

@klueska @jgehrcke what are your thoughts on the UX?

Copilot

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",

When running the nvidia-ctk cdi generate command, a user should be able to opt out of specific hooks. We propose to add a flag --disable-hook that will take a comma-separated list of hooks that will be skipped when creating the CDI spec. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

elezar · 2025-05-14T13:29:35Z

cmd/nvidia-ctk/cdi/generate/generate.go

+
+	if len(opts.disableHooks.Value()) > 0 {
+		for _, hook := range opts.disableHooks.Value() {
+			if hook == "all" {


I think all should be a valid hook name implemented in the API. For how I could imagine this working, see the way we handle NVIDIA_VISIBLE_DEVICES in internal/config/image/devices.go.

Thanks for the idea, PTAL

elezar · 2025-05-15T09:28:46Z

pkg/nvcdi/api.go

 )
+
+// NewHookName takes a string and returns a []HookName, empty if the HookName


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?

elezar · 2025-05-15T09:31:55Z

cmd/nvidia-ctk/cdi/generate/generate.go

+	if len(opts.disableHooks.Value()) > 0 {
+		for _, hook := range opts.disableHooks.Value() {
+			for _, hookName := range nvcdi.NewHookName(hook) {
+				initOpts = append(initOpts, nvcdi.WithDisabledHook(hookName))
+			}
+		}
+	}


The len check is not required. One can always iterate over an empty slice.

Suggested change

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.

Suggestion taken

elezar · 2025-05-15T09:32:10Z

cmd/nvidia-ctk/cdi/generate/generate.go

@@ -57,6 +57,7 @@ type options struct {

 	configSearchPaths  cli.StringSlice
 	librarySearchPaths cli.StringSlice
+	disableHooks       cli.StringSlice


Suggested change

disableHooks cli.StringSlice

disabledHooks cli.StringSlice

elezar · 2025-05-15T09:34:27Z

pkg/nvcdi/driver-nvml.go

-		l.nvidiaCDIHookPath,
-	)
-	discoverers = append(discoverers, driverDotSoSymlinksDiscoverer)
+	if l.HookIsSupported(HookCreateSymlinks) {


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.

klueska · 2025-05-15T11:00:31Z

cmd/nvidia-ctk/cdi/generate/generate.go

@@ -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:        "disable-hook",


Should we also add a disable-all-hooks flag so people don't have to enumerate all of them to disable them.

Also, should we add a list-hooks command so people can discover what hooks are available for disabling

@klueska my thoughts on how to handle the disabling of all hooks is to also accept all as a valid hook name. So that --disable-hook(s)=all can be specified.

Also, should we add a list-hooks command so people can discover what hooks are available for disabling

Do you mean something like:

nvidia-ctk cdi list-hooks

I have added an alias of disable-hooks and a better Usage description.

Copilot

Pull Request Overview

This PR introduces the --disable-hook flag for the CDI generate command so that users can opt out of specific hooks when creating the CDI spec. Key changes include:

Updating the options API to support disabling hooks via a generic WithDisabledHook function.
Adding a Set method to the disabledHooks type to handle the "all" keyword.
Integrating the new flag into the CLI command and updating tests to validate its behavior.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/nvcdi/options.go	Updated WithDisabledHook to accept both string and HookName types.
pkg/nvcdi/hooks.go	Added a Set method on disabledHooks to support the "all" keyword.
pkg/nvcdi/driver-nvml.go	Wrapped hook discovery calls with HookIsSupported checks.
pkg/nvcdi/api.go	Introduced new hook definitions and an AllHooks list.
cmd/nvidia-ctk/cdi/generate/generate.go	Added the disable-hook flag to the CLI command and injected the disabledHooks option.
cmd/nvidia-ctk/cdi/generate/generate_test.go	Extended tests to check disabling hooks including the special "all" value.

Comments suppressed due to low confidence (1)

pkg/nvcdi/options.go:159

Fix the typo in the comment to read 'allows specific hooks to be disabled.'

// WithDisabledHook allows specific hooks to the disabled.

pkg/nvcdi/hooks.go

pkg/nvcdi/api.go

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez · 2025-05-16T19:34:27Z

PR #1090 will refactor how we handle the Hook Creation, and after it, I'll update this PR to build on that and provide a better handling of disabled hooks via flags

ArangoGutierrez requested review from elezar and Copilot May 12, 2025 16:53

ArangoGutierrez self-assigned this May 12, 2025

Copilot AI reviewed May 12, 2025

View reviewed changes

ArangoGutierrez force-pushed the 1074 branch from d9c00f7 to ef09bbf Compare May 12, 2025 17:59

ArangoGutierrez requested a review from Copilot May 12, 2025 18:00

Copilot AI reviewed May 12, 2025

View reviewed changes

cmd/nvidia-ctk/cdi/generate/generate.go Outdated Show resolved Hide resolved

ArangoGutierrez force-pushed the 1074 branch from ef09bbf to 2b37211 Compare May 12, 2025 18:15

ArangoGutierrez requested a review from Copilot May 12, 2025 18:15

Copilot AI reviewed May 12, 2025

View reviewed changes

cmd/nvidia-ctk/cdi/generate/generate.go Show resolved Hide resolved

elezar reviewed May 13, 2025

View reviewed changes

cmd/nvidia-ctk/cdi/generate/generate.go Outdated Show resolved Hide resolved

elezar reviewed May 13, 2025

View reviewed changes

cmd/nvidia-ctk/cdi/generate/generate.go Outdated Show resolved Hide resolved

elezar reviewed May 13, 2025

View reviewed changes

ArangoGutierrez force-pushed the 1074 branch from 2b37211 to 194c1e6 Compare May 14, 2025 09:18

ArangoGutierrez changed the title ~~Add --skip-hooks flag to cdi generate command~~ Add --disable-hook flag to cdi generate command May 14, 2025

ArangoGutierrez requested review from elezar and Copilot May 14, 2025 10:09

Copilot AI reviewed May 14, 2025

View reviewed changes

ArangoGutierrez force-pushed the 1074 branch from b6e5911 to 64375df Compare May 14, 2025 10:36

ArangoGutierrez added 2 commits May 14, 2025 12:36

rename flag to disable-hooks

d657e0a

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez force-pushed the 1074 branch from 64375df to b32d8a9 Compare May 14, 2025 10:37

elezar reviewed May 14, 2025

View reviewed changes

ArangoGutierrez force-pushed the 1074 branch from b32d8a9 to b6ac316 Compare May 14, 2025 14:13

ArangoGutierrez requested a review from elezar May 14, 2025 14:14

elezar reviewed May 15, 2025

View reviewed changes

klueska reviewed May 15, 2025

View reviewed changes

ArangoGutierrez force-pushed the 1074 branch from b6ac316 to 9c3a26b Compare May 15, 2025 15:27

ArangoGutierrez requested review from klueska, elezar and Copilot May 15, 2025 15:27

Copilot AI reviewed May 15, 2025

View reviewed changes

pkg/nvcdi/hooks.go Show resolved Hide resolved

pkg/nvcdi/api.go Show resolved Hide resolved

Add all option to disable all hooks

191f11e

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez force-pushed the 1074 branch from 9c3a26b to 191f11e Compare May 15, 2025 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --disable-hook flag to cdi generate command #1077

Add --disable-hook flag to cdi generate command #1077

ArangoGutierrez commented May 12, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI left a comment

Copilot AI left a comment

elezar May 13, 2025

ArangoGutierrez May 13, 2025

elezar May 13, 2025

ArangoGutierrez May 14, 2025

elezar May 13, 2025

elezar May 14, 2025

Copilot AI left a comment

elezar May 14, 2025

ArangoGutierrez May 14, 2025

elezar May 15, 2025

ArangoGutierrez May 15, 2025

elezar May 15, 2025 •

edited

Loading

ArangoGutierrez May 15, 2025

elezar May 15, 2025

ArangoGutierrez May 15, 2025

elezar May 15, 2025

klueska May 15, 2025

elezar May 15, 2025

elezar May 15, 2025 •

edited

Loading

ArangoGutierrez May 15, 2025

Copilot AI left a comment

ArangoGutierrez commented May 16, 2025

		)

		// NewHookName takes a string and returns a []HookName, empty if the HookName

Add --disable-hook flag to cdi generate command #1077

Are you sure you want to change the base?

Add --disable-hook flag to cdi generate command #1077

Conversation

ArangoGutierrez commented May 12, 2025 • edited Loading

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar May 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

ArangoGutierrez commented May 16, 2025

ArangoGutierrez commented May 12, 2025 •

edited

Loading

elezar May 15, 2025 •

edited

Loading

elezar May 15, 2025 •

edited

Loading