Issue warning on unsupported CDI hook #1000

elezar · 2025-03-19T15:05:12Z

To allow for CDI hooks to be added gradually we provide a generic no-op hook for unrecognised subcommands. This will log a warning (to STDERR) instead of erroring out.

For example:

$ ./nvidia-cdi-hook foo
WARN[0000] Unsupported CDI hook: foo
$ echo $?
0
$ ./nvidia-cdi-hook
WARN[0000] No CDI hook specified
$ echo $?
0

./nvidia-ctk hook foo
WARN[0000] Unsupported CDI hook: foo
$ echo $?
0
./nvidia-ctk hook
WARN[0000] No CDI hook specified
$ echo $?
0

Copilot

Pull Request Overview

This PR introduces a generic no-op hook that logs a warning for unsupported subcommands, allowing CDI hooks to be added gradually without causing errors.

Added WarnOnUnsupportedSubcommand functions in both CDI hook and CTK hook implementations
Updated the main action for CDI hook and the hook command action for CTK to use the new warning function

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
cmd/nvidia-cdi-hook/commands/commands.go	Added a new helper function to log a warning for unsupported subcommands
cmd/nvidia-ctk/hook/hook.go	Updated the hook command to call the warning function when an unsupported subcommand is invoked
cmd/nvidia-cdi-hook/main.go	Configured the CDI hook main command to warn when no valid subcommand is provided

Comments suppressed due to low confidence (2)

cmd/nvidia-ctk/hook/hook.go:44

[nitpick] The inline Action definition is clear now; if additional similar actions are added in other hooks, consider centralizing this behavior to reduce code duplication.

Action: func(ctx *cli.Context) error {

cmd/nvidia-cdi-hook/main.go:55

[nitpick] The Action hook configuration aligns with the generic no-op behavior; ensure that any future extensions continue to follow this consistent pattern.

c.Action = func(ctx *cli.Context) error {

cmd/nvidia-cdi-hook/commands/commands.go

jgehrcke · 2025-03-21T13:02:42Z

cmd/nvidia-ctk/hook/hook.go

@@ -41,6 +41,9 @@ func (m hookCommand) build() *cli.Command {
 	hook := cli.Command{
 		Name:  "hook",


From the CLI libraries perspective -- does this define a subcommand named "hook"?

if so, the code comment maybe should be "create 'hook' subcommand" (just noting this as I try to understand the code here)

The CLI library that we use (urfave) has the following heirarchy:
A top-level CLI App can have one or more Commands. Each Command can have one or more Subcommands.

Note that The Subcommands are themselves Commands which actually allows for arbitrary nesting.

I agree that from a documentation perspective, what we mean when we say "command" is "subcommand" and should use "command" for "app" (the top-level CLI) instead.

jgehrcke · 2025-03-21T13:03:51Z

cmd/nvidia-ctk/hook/hook.go

@@ -41,6 +41,9 @@ func (m hookCommand) build() *cli.Command {
 	hook := cli.Command{
 		Name:  "hook",
 		Usage: "A collection of hooks that may be injected into an OCI spec",
+		Action: func(ctx *cli.Context) error {
+			return commands.WarnOnUnsupportedSubcommand(m.logger, ctx)
+		},
 	}

 	hook.Subcommands = commands.New(m.logger)


oh. Hm maybe "hook" is the command, and sub-commands are one level deeper.

I hope my discussion above has cleared up the confusion here. We should make a pass of the existing code and update the comments / functions for consistency, but this has a low priority at present.

jgehrcke · 2025-03-21T13:09:46Z

cmd/nvidia-cdi-hook/main.go

@@ -51,6 +51,11 @@ func main() {
 	c.Usage = "Command to structure files for usage inside a container, called as hooks from a container runtime, defined in a CDI yaml file"
 	c.Version = info.GetVersionString()

+	// We add an action to the hook


The comment "We add an action to the hook" does not help me understand the code better :).

I think maybe instead of "We add an action to the hook" we can write "Add a default action to this CLI. It is executed when the user-provided sub-command is unknown"

I still wonder about the hierarchy provided by this CLI library. There is a CLI. The CLI has various commands. Each command has subcommands. Is that true? Or is the CLI the command?

I ask because I wonder: what is a hook? A command? A subcommand?

Again, this is about code readability. I have built CLIs with various different libraries and ecosystems and I know everyone calls things differently... :)

Now that I see

./nvidia-cdi-hook foo

and

./nvidia-ctk hook foo

I realize that we might have both: hook being the CLI, and hook also being a command in a CLI.. ouch. OK.

Yes, a hook as a general concept is an executable that is run by a low-level runtime at various points of a container's lifecycle. These are passed the container state via STDIN which we use to extract the information about the container -- for example the container root or even the environment variables in the container in the case of the nvidia-container-runtime-hook.

For CDI we have a number of smaller hooks that are supposed to do "one" thing (or at least have their intent clear from their name). We originally implemented these as subcommands of the hook subcommand of the nvidia-ctk command so as to simplify the distribution of the executable. In order to simplify discussions around hooks and what their intent are, we later decided to implement a separate command nvidia-cdi-hook that implements the same hooks as subcommands. With this done the following are equivalent:

./nvidia-cdi-hook [HOOK_NAME] ./nvidia-ctk hook [HOOK_NAME]

As I also mentioned on the call, we could take this further and implement separate executables for each of the hooks. This does have the disadvantage that we would not be able to implement "default" behaviour as we are doing here.

jgehrcke · 2025-03-21T13:10:40Z

cmd/nvidia-cdi-hook/main.go

@@ -51,6 +51,11 @@ func main() {
 	c.Usage = "Command to structure files for usage inside a container, called as hooks from a container runtime, defined in a CDI yaml file"
 	c.Version = info.GetVersionString()

+	// We add an action to the hook
+	c.Action = func(ctx *cli.Context) error {
+		return commands.WarnOnUnsupportedSubcommand(logger, ctx)


I suppose a decisive design decision here is "emit warning on stderr, and exit 0".

Is that right? I think we should maybe document that via code comment.

Strictly speaking where the error is emitted to is determined by the logger. The default logger as implemented should go to STDERR though. I have updated the code to make it clearer that we also return no error here.

jgehrcke

Thank you @elezar! Merge this at will. We talked about offline, and of course it's ready to land.

In terms of code readability I have however left a few questions / remarks / thoughts. Please reply and/or adjust code at your own discretion! :)

jgehrcke · 2025-03-21T13:13:49Z

WARN[0000] Unsupported hook &[foo]

does this get emitted to stderr?
why do we see &[foo] and not "foo"?

elezar · 2025-04-02T13:08:37Z

WARN[0000] Unsupported hook &[foo]
does this get emitted to stderr?

why do we see &[foo] and not "foo"?

The standard logger that's used (logrus.New()) will print this to STDERR. The output is a result of how go is converting the arg(s) to a string for output.

To allow for CDI hooks to be added gradually we provide a generic no-op hook for unrecognised subcommands. This will log a warning instead of erroring out. An unsupported hook could be the result of a CDI specification referring to a new hook that is not yet supported by an older NVIDIA Container Toolkit version or a hook that has been removed in newer version. Signed-off-by: Evan Lezar <[email protected]>

elezar added this to the v1.18.0 milestone Mar 19, 2025

ArangoGutierrez approved these changes Mar 20, 2025

View reviewed changes

ArangoGutierrez requested a review from Copilot March 20, 2025 09:52

Copilot AI reviewed Mar 20, 2025

View reviewed changes

cmd/nvidia-cdi-hook/commands/commands.go Outdated Show resolved Hide resolved

elezar requested a review from klueska March 20, 2025 11:01

jgehrcke reviewed Mar 21, 2025

View reviewed changes

jgehrcke approved these changes Mar 21, 2025

View reviewed changes

elezar force-pushed the ignore-unknown-hooks branch from c31427f to 1b68cb0 Compare April 2, 2025 13:36

elezar force-pushed the ignore-unknown-hooks branch from 1b68cb0 to b3ad04d Compare April 2, 2025 14:28

elezar requested a review from jgehrcke April 17, 2025 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue warning on unsupported CDI hook #1000

Issue warning on unsupported CDI hook #1000

elezar commented Mar 19, 2025 •

edited

Loading

Copilot AI left a comment

jgehrcke Mar 21, 2025

jgehrcke Mar 21, 2025

elezar Apr 2, 2025

jgehrcke Mar 21, 2025

elezar Apr 4, 2025

jgehrcke Mar 21, 2025

jgehrcke Mar 21, 2025

elezar Apr 2, 2025

jgehrcke Mar 21, 2025

elezar Apr 4, 2025

jgehrcke left a comment

jgehrcke commented Mar 21, 2025

elezar commented Apr 2, 2025

		@@ -41,6 +41,9 @@ func (m hookCommand) build() *cli.Command {
		hook := cli.Command{
		Name: "hook",

Issue warning on unsupported CDI hook #1000

Are you sure you want to change the base?

Issue warning on unsupported CDI hook #1000

Conversation

elezar commented Mar 19, 2025 • edited Loading

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

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

jgehrcke left a comment

Choose a reason for hiding this comment

jgehrcke commented Mar 21, 2025

elezar commented Apr 2, 2025

elezar commented Mar 19, 2025 •

edited

Loading