Merge pull request #1221 from ArangoGutierrez/i-1218

Disable chmod-hook by default

Author: elezar

cmd/nvidia-ctk-installer/toolkit/toolkit_test.go

@@ -84,6 +84,8 @@ devices:
               hostPath: /host/driver/root/dev/nvidia-caps-imex-channels/channel1
             - path: /dev/nvidia-caps-imex-channels/channel2047
               hostPath: /host/driver/root/dev/nvidia-caps-imex-channels/channel2047
+            - path: /dev/nvidia-caps/nvidia-cap1
+              hostPath: /host/driver/root/dev/nvidia-caps/nvidia-cap1
 containerEdits:
     env:
         - NVIDIA_CTK_LIBCUDA_DIR=/lib/x86_64-linux-gnu

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

@@ -62,6 +62,7 @@ type options struct {
 	configSearchPaths  []string
 	librarySearchPaths []string
 	disabledHooks      []string
+	enabledHooks       []string
 
 	csv struct {
 		files          []string
@@ -214,6 +215,13 @@ func (m command) build() *cli.Command {
 				Destination: &opts.disabledHooks,
 				Sources:     cli.EnvVars("NVIDIA_CTK_CDI_GENERATE_DISABLED_HOOKS"),
 			},
+			&cli.StringSliceFlag{
+				Name:        "enable-hook",
+				Aliases:     []string{"enable-hooks"},
+				Usage:       "Explicitly enable a hook in the generated CDI specification. This overrides disabled hooks. This can be specified multiple times.",
+				Destination: &opts.enabledHooks,
+				Sources:     cli.EnvVars("NVIDIA_CTK_CDI_GENERATE_ENABLED_HOOKS"),
+			},
 		},
 	}
 
@@ -258,6 +266,12 @@ func (m command) validateFlags(c *cli.Command, opts *options) error {
 	if err := cdi.ValidateClassName(opts.class); err != nil {
 		return fmt.Errorf("invalid CDI class name: %v", err)
 	}
+
+	for _, hook := range opts.enabledHooks {
+		if hook == "all" {
+			return fmt.Errorf("enabling all hooks is not supported")
+		}
+	}
 	return nil
 }
 
@@ -313,14 +327,12 @@ func (m command) generateSpec(opts *options) (spec.Interface, error) {
 		nvcdi.WithLibrarySearchPaths(opts.librarySearchPaths),
 		nvcdi.WithCSVFiles(opts.csv.files),
 		nvcdi.WithCSVIgnorePatterns(opts.csv.ignorePatterns),
+		nvcdi.WithDisabledHooks(opts.disabledHooks...),
+		nvcdi.WithEnabledHooks(opts.enabledHooks...),
 		// We set the following to allow for dependency injection:
 		nvcdi.WithNvmlLib(opts.nvmllib),
 	}
 
-	for _, hook := range opts.disabledHooks {
-		cdiOptions = append(cdiOptions, nvcdi.WithDisabledHook(hook))
-	}
-
 	cdilib, err := nvcdi.New(cdiOptions...)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create CDI library: %v", err)

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

@@ -359,6 +359,91 @@ containerEdits:
             - nodev
             - rbind
             - rprivate
+`,
+		},
+		{
+			description: "enableChmodHook",
+			options: options{
+				format:        "yaml",
+				mode:          "management",
+				vendor:        "example.com",
+				class:         "device",
+				driverRoot:    driverRoot,
+				enabledHooks:  []string{"chmod"},
+				disabledHooks: []string{"enable-cuda-compat", "update-ldcache", "disable-device-node-modification"},
+			},
+			expectedOptions: options{
+				format:            "yaml",
+				mode:              "management",
+				vendor:            "example.com",
+				class:             "device",
+				nvidiaCDIHookPath: "/usr/bin/nvidia-cdi-hook",
+				driverRoot:        driverRoot,
+				enabledHooks:      []string{"chmod"},
+				disabledHooks:     []string{"enable-cuda-compat", "update-ldcache", "disable-device-node-modification"},
+			},
+			expectedSpec: `---
+cdiVersion: 0.5.0
+kind: example.com/device
+devices:
+    - name: all
+      containerEdits:
+        deviceNodes:
+            - path: /dev/nvidia0
+              hostPath: {{ .driverRoot }}/dev/nvidia0
+            - path: /dev/nvidiactl
+              hostPath: {{ .driverRoot }}/dev/nvidiactl
+            - path: /dev/nvidia-caps-imex-channels/channel0
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps-imex-channels/channel0
+            - path: /dev/nvidia-caps-imex-channels/channel1
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps-imex-channels/channel1
+            - path: /dev/nvidia-caps-imex-channels/channel2047
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps-imex-channels/channel2047
+            - path: /dev/nvidia-caps/nvidia-cap1
+              hostPath: {{ .driverRoot }}/dev/nvidia-caps/nvidia-cap1
+        hooks:
+            - hookName: createContainer
+              path: /usr/bin/nvidia-cdi-hook
+              args:
+                - nvidia-cdi-hook
+                - chmod
+                - --mode
+                - "755"
+                - --path
+                - /dev/nvidia-caps
+              env:
+                - NVIDIA_CTK_DEBUG=false
+containerEdits:
+    env:
+        - NVIDIA_CTK_LIBCUDA_DIR=/lib/x86_64-linux-gnu
+        - NVIDIA_VISIBLE_DEVICES=void
+    hooks:
+        - hookName: createContainer
+          path: /usr/bin/nvidia-cdi-hook
+          args:
+            - nvidia-cdi-hook
+            - create-symlinks
+            - --link
+            - libcuda.so.1::/lib/x86_64-linux-gnu/libcuda.so
+          env:
+            - NVIDIA_CTK_DEBUG=false
+    mounts:
+        - hostPath: {{ .driverRoot }}/lib/x86_64-linux-gnu/libcuda.so.999.88.77
+          containerPath: /lib/x86_64-linux-gnu/libcuda.so.999.88.77
+          options:
+            - ro
+            - nosuid
+            - nodev
+            - rbind
+            - rprivate
+        - hostPath: {{ .driverRoot }}/lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia.so.999.88.77
+          containerPath: /lib/x86_64-linux-gnu/vdpau/libvdpau_nvidia.so.999.88.77
+          options:
+            - ro
+            - nosuid
+            - nodev
+            - rbind
+            - rprivate
 `,
 		},
 	}

internal/discover/hooks.go

@@ -50,6 +50,14 @@ const (
 	defaultNvidiaCDIHookPath = "/usr/bin/nvidia-cdi-hook"
 )
 
+// defaultDisabledHooks defines hooks that are disabled by default.
+// These hooks can be explicitly enabled using the WithEnabledHooks option.
+var defaultDisabledHooks = []HookName{
+	// ChmodHook is disabled by default as it was a workaround for older
+	// versions of crun that has since been fixed.
+	ChmodHook,
+}
+
 var _ Discover = (*Hook)(nil)
 
 // Devices returns an empty list of devices for a Hook discoverer.
@@ -77,7 +85,14 @@ func (h *Hook) Hooks() ([]Hook, error) {
 	return []Hook{*h}, nil
 }
 
-type Option func(*cdiHookCreator)
+type hookCreatorOptions struct {
+	nvidiaCDIHookPath string
+	disabledHooks     []HookName
+	enabledHooks      []HookName
+	debugLogging      bool
+}
+
+type Option func(*hookCreatorOptions)
 
 type cdiHookCreator struct {
 	nvidiaCDIHookPath string
@@ -100,39 +115,66 @@ type HookCreator interface {
 	Create(HookName, ...string) *Hook
 }
 
-// WithDisabledHooks sets the set of hooks that are disabled for the CDI hook creator.
+func WithDebugLogging(debugLogging bool) Option {
+	return func(hco *hookCreatorOptions) {
+		hco.debugLogging = debugLogging
+	}
+}
+
+// WithDisabledHooks explicitly disables the specified hooks.
 // This can be specified multiple times.
 func WithDisabledHooks(hooks ...HookName) Option {
-	return func(c *cdiHookCreator) {
-		for _, hook := range hooks {
-			c.disabledHooks[hook] = true
-		}
+	return func(c *hookCreatorOptions) {
+		c.disabledHooks = append(c.disabledHooks, hooks...)
+	}
+}
+
+// WithEnabledHooks explicitly enables the specified hooks.
+// This is useful for enabling hooks that are disabled by default.
+func WithEnabledHooks(hooks ...HookName) Option {
+	return func(c *hookCreatorOptions) {
+		c.enabledHooks = append(c.enabledHooks, hooks...)
 	}
 }
 
 // WithNVIDIACDIHookPath sets the path to the nvidia-cdi-hook binary.
 func WithNVIDIACDIHookPath(nvidiaCDIHookPath string) Option {
-	return func(c *cdiHookCreator) {
+	return func(c *hookCreatorOptions) {
 		c.nvidiaCDIHookPath = nvidiaCDIHookPath
 	}
 }
 
 func NewHookCreator(opts ...Option) HookCreator {
-	cdiHookCreator := &cdiHookCreator{
+	o := &hookCreatorOptions{
 		nvidiaCDIHookPath: defaultNvidiaCDIHookPath,
-		disabledHooks:     make(map[HookName]bool),
 	}
 	for _, opt := range opts {
-		opt(cdiHookCreator)
+		opt(o)
 	}
 
-	if cdiHookCreator.disabledHooks[AllHooks] {
+	o.disabledHooks = append(o.disabledHooks, defaultDisabledHooks...)
+
+	disabledHooks := make(map[HookName]bool)
+	for _, h := range o.disabledHooks {
+		disabledHooks[h] = true
+	}
+
+	if disabledHooks[AllHooks] && len(o.enabledHooks) == 0 {
 		return &allDisabledHookCreator{}
 	}
 
-	cdiHookCreator.fixedArgs = getFixedArgsForCDIHookCLI(cdiHookCreator.nvidiaCDIHookPath)
+	for _, h := range o.enabledHooks {
+		disabledHooks[h] = false
+	}
+
+	c := &cdiHookCreator{
+		nvidiaCDIHookPath: o.nvidiaCDIHookPath,
+		disabledHooks:     disabledHooks,
+		fixedArgs:         getFixedArgsForCDIHookCLI(o.nvidiaCDIHookPath),
+		debugLogging:      o.debugLogging,
+	}
 
-	return cdiHookCreator
+	return c
 }
 
 // Create creates a new hook with the given name and arguments.
@@ -150,21 +192,19 @@ func (c cdiHookCreator) Create(name HookName, args ...string) *Hook {
 	}
 }
 
-// isDisabled checks if the specified hook name is disabled.
 func (c cdiHookCreator) isDisabled(name HookName, args ...string) bool {
-	if c.disabledHooks[name] {
+	disabled, ok := c.disabledHooks[name]
+	if ok {
+		return disabled
+	}
+	if c.disabledHooks[AllHooks] {
 		return true
 	}
 
+	// still reject hooks that require args if none were provided
 	switch name {
-	case CreateSymlinksHook:
-		if len(args) == 0 {
-			return true
-		}
-	case ChmodHook:
-		if len(args) == 0 {
-			return true
-		}
+	case CreateSymlinksHook, ChmodHook:
+		return len(args) == 0
 	}
 	return false
 }