Add the ability to disable specific (or all) CDI hooks when generating a CDI specification #1077

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

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

nvidia-ctk cdi generate \
    --disable-hook=enable-cuda-compat \
    --disable-hook=update-ldcache

Specifying:

nvidia-ctk cdi generate \
    --disable-hooks=all

(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 option WithDisabledHook("enable-cuda-compat") or WithDisabledHook("all") can be used. (Constants were added for supported hooks).

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

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

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.

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

elezar · 2025-05-23T08:30:10Z

internal/discover/hooks.go

 }

-func NewHookCreator(nvidiaCDIHookPath string) HookCreator {
+func NewHookCreator(nvidiaCDIHookPath string, disabledHooks DisabledHooks) HookCreator {


What about using:

Suggested change

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.

instead I used the options route, let me know what you think

Looks good. I have added a commit on top that also sets the hook path as an option for consistency.

elezar · 2025-05-23T08:33:17Z

internal/discover/hooks.go

+func (d DisabledHooks) Set(value HookName) {
+	if value == "all" {
+		for _, hook := range AllHooks {
+			d[hook] = true
+		}
+		return
+	}
+
+	d[value] = true
+}


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] }

func removed, it was not needed by moving to a options implementation

elezar · 2025-06-03T04:24:07Z

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

@@ -270,7 +270,7 @@ func (m command) generateSpec(opts *options) (spec.Interface, error) {
 		deviceNamers = append(deviceNamers, deviceNamer)
 	}

-	initOpts := []nvcdi.Option{
+	cdiOptions := []nvcdi.Option{


nit: initOptions was too generic.

elezar · 2025-06-03T04:25:14Z

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

@@ -127,7 +124,7 @@ containerEdits:
 				vendor:        "example.com",
 				class:         "device",
 				driverRoot:    driverRoot,
-				disabledHooks: *disableHook1,
+				disabledHooks: valueOf(cli.NewStringSlice("enable-cuda-compat")),


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.

elezar · 2025-06-03T04:29:39Z

internal/discover/hooks.go

+// A HookName represents a supported CDI hooks.
+type HookName string
+
+const (


I made the following changes:

moved the constants earlier in the file as this is common practice in golang

renamed the types *Hook instead of Hook*. 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.

elezar · 2025-06-03T04:29:58Z

internal/discover/hooks.go

-	// cache inside the directory path to be mounted into a container.
-	HookUpdateLDCache = HookName("update-ldcache")
-)
+type cdiHookCreator struct {


I made the type private and renamed it for clarity.

elezar · 2025-06-03T04:30:17Z

internal/discover/hooks.go

-type HookName string
-
-// disabledHooks allows individual hooks to be disabled.
-type disabledHooks map[HookName]bool


The type was not used since we were not attaching methods to it.

elezar · 2025-06-03T04:30:54Z

internal/discover/hooks.go

-	HookEnableCudaCompat,
-	HookCreateSymlinks,
-	HookUpdateLDCache,
+	fixedArgs []string


Instead of checking the path on every Create invocation, we check it once at construction.

elezar · 2025-06-03T04:32:32Z

internal/discover/hooks.go

 }

-type Option func(*CDIHook)
+// An allDisabledHookCreator is a HookCreator that does not create any hooks.


Instead of checking disabledHooks on every call to Create, we instantiate a type that creates no hooks. (This is the null object pattern).

elezar · 2025-06-03T04:33:39Z

internal/discover/hooks.go

+}
+
+// isDisabled checks if the specified hook name is disabled.
+func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool {


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.

elezar · 2025-06-03T04:33:58Z

internal/discover/hooks.go

+	return append(c.fixedArgs, string(name))
+}
+
+func (c cdiHookCreator) transformArgs(name HookName, args ...string) []string {


Added a function to handle the per-hook transforms.

elezar · 2025-06-03T04:34:18Z

internal/discover/hooks.go

+}
+
+// isDisabled checks if the specified hook name is disabled.
+func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool {


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.

elezar · 2025-06-03T04:35:05Z

internal/discover/symlinks_test.go

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


Using a specific path here wasn't adding value. Changed the tests to check the default.

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]>

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

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 2 times, most recently from 64375df to b32d8a9 Compare May 14, 2025 10:37

elezar reviewed May 14, 2025

View reviewed changes

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

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

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

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

elezar reviewed May 23, 2025

View reviewed changes

ArangoGutierrez requested a review from elezar May 23, 2025 09:08

ArangoGutierrez force-pushed the 1074 branch 2 times, most recently from 9aa88ea to f72732e Compare May 23, 2025 09:12

elezar reviewed Jun 3, 2025

View reviewed changes

elezar force-pushed the 1074 branch 3 times, most recently from c6428c1 to caf855b Compare June 3, 2025 04:43

ArangoGutierrez force-pushed the 1074 branch from caf855b to d1e58c7 Compare June 3, 2025 12:09

elezar changed the title ~~Add --disable-hook flag to cdi generate command~~ Add the ability to disable specific (or all) CDI hooks Jun 3, 2025

elezar changed the title ~~Add the ability to disable specific (or all) CDI hooks~~ Add the ability to disable specific (or all) CDI hooks when generating a CDI specification Jun 3, 2025

Consolidate HookName functionality on internal/discover pkg

b478751

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

ArangoGutierrez force-pushed the 1074 branch from d1e58c7 to 9f06e13 Compare June 3, 2025 13:24

elezar force-pushed the 1074 branch from 9f06e13 to 73f9cc8 Compare June 3, 2025 13:56

elezar force-pushed the 1074 branch from 73f9cc8 to 6cf0248 Compare June 3, 2025 14:01

elezar approved these changes Jun 3, 2025

View reviewed changes

elezar merged commit d59fd3d into NVIDIA:main Jun 3, 2025
13 checks passed

ArangoGutierrez added this to the v1.18.0 milestone Jun 3, 2025

		)

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

	func NewHookCreator(nvidiaCDIHookPath string, disabledHooks DisabledHooks) HookCreator {
	func NewHookCreator[T string\|HookName](nvidiaCDIHookPath string, disabledHooks ...T) HookCreator {

Add the ability to disable specific (or all) CDI hooks when generating a CDI specification #1077

Add the ability to disable specific (or all) CDI hooks when generating a CDI specification #1077

Uh oh!

Conversation

ArangoGutierrez commented May 12, 2025 • edited by elezar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elezar May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elezar May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArangoGutierrez commented May 12, 2025 •

edited by elezar

Loading

elezar May 15, 2025 •

edited

Loading

elezar May 23, 2025 •

edited

Loading