Fix handling of unrecognised CDI hooks #1308

ArangoGutierrez · 2025-09-22T09:26:53Z

Previously, when an unrecognized CDI hook was invoked with command-line flags, the nvidia-cdi-hook command would fail with a flag parsing error instead of gracefully handling the unsupported hook. This could cause container launch failures when CDI specifications reference hooks that are not yet supported by the current NVIDIA Container Toolkit version.

This PR enhances the error handling in nvidia-cdi-hook to gracefully handle unrecognized CDI hooks that include command-line flags. The changes ensure that:
Unrecognized hooks with flags are handled gracefully - Instead of failing with flag parsing errors, the command now issues a warning and continues
Backwards compatibility is maintained - Existing behavior for supported hooks remains unchanged
Container launches don't fail - Unsupported hooks result in warnings rather than errors
Changes
Added OnUsageError handler in ConfigureCDIHookCommand to detect flag parsing errors for unrecognized commands
Added strings import for error message parsing to identify flag-related errors
Enhanced error detection to specifically catch "flag provided but not defined" errors for unrecognized hooks
Improved warning consistency by reusing the existing IssueUnsupportedHookWarning logic

Test Cases

The following test cases demonstrate the improved behavior:

Before (failing):

$ ./nvidia-cdi-hook unknown-hook --some-flag value
Error: flag provided but not defined: -some-flag

After (graceful handling):

$ ./nvidia-cdi-hook unknown-hook --some-flag value
time="2025-09-22T11:25:26+02:00" level=warning msg="Unsupported CDI hook: unknown-hook"

Additional test cases:

# Unrecognized hook without flags
$ ./nvidia-cdi-hook unknown-hook
time="2025-09-22T11:25:26+02:00" level=warning msg="Unsupported CDI hook: unknown-hook"

# No hook specified
$ ./nvidia-cdi-hook
time="2025-09-22T11:25:26+02:00" level=warning msg="No CDI hook specified"

# Recognized hook with valid flags (unchanged behavior)
$ ./nvidia-cdi-hook update-ldcache --debug
# Works as expected, no warning

# Recognized hook with invalid flags (unchanged behavior)
$ ./nvidia-cdi-hook update-ldcache --invalid-flag
Error: flag provided but not defined: -invalid-flag

Copilot

Pull Request Overview

This PR adds warning functionality for unrecognized CDI hooks that include flags, addressing issue #1225. The change enhances error handling to catch cases where unknown hooks are invoked with command-line flags.

Adds a OnUsageError handler to detect unrecognized hooks with flags
Imports the strings package for string manipulation in error checking

cmd/nvidia-cdi-hook/main.go

coveralls · 2025-09-22T09:29:16Z

Pull Request Test Coverage Report for Build 18008895528

Details

0 of 62 (0.0%) changed or added relevant lines in 3 files are covered.
5 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 36.168%

Changes Missing Coverage	Changed/Added Lines	%
cmd/nvidia-ctk/hook/hook.go	2	0.0%
cmd/nvidia-cdi-hook/main.go	28	0.0%
cmd/nvidia-cdi-hook/commands/commands.go	32	0.0%

Files with Coverage Reduction	New Missed Lines	%
cmd/nvidia-cdi-hook/main.go	5	0.0%

Totals
Change from base Build 17981864462:	-0.1%
Covered Lines:	4827
Relevant Lines:	13346

💛 - Coveralls

ArangoGutierrez · 2025-09-22T09:29:37Z

Tested

./nvidia-cdi-hook unknown-hook --some-flag value 
time="2025-09-22T11:25:26+02:00" level=warning msg="Unsupported CDI hook: unknown-hook"

elezar · 2025-09-24T08:52:47Z

cmd/nvidia-cdi-hook/main.go

+		OnUsageError: func(ctx context.Context, cmd *cli.Command, err error, isSubcommand bool) error {
+			// Check if this is a "flag provided but not defined" error
+			errMsg := err.Error()
+			if strings.HasPrefix(errMsg, "flag provided but not defined: ") {


One nit: The err in urfave is defined as:

providedButNotDefinedErrMsg = "flag provided but not defined: -"

and we don't include the - here.

ah yeah, let me edit!

elezar

What about the case of a subcommand that does not accept arguments:

$ ./nvidia-cdi-hook foo
No help topic for 'foo'
$ echo $?
3

Copilot

Pull Request Overview

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

cmd/nvidia-cdi-hook/main.go

Changes addressed

elezar · 2025-09-24T12:00:24Z

cmd/nvidia-cdi-hook/main.go

+		OnUsageError: func(ctx context.Context, cmd *cli.Command, err error, isSubcommand bool) error {
+			errMsg := err.Error()
+
+			// Check if this is a "No help topic for" error (unrecognized


Since in Urfave we have a CommandNotFound handler does it not make sense to handle this separately?

elezar

One thing that I forgot to mention is that we would also have to update the nvidia-ctk hook command.

elezar · 2025-09-24T12:39:14Z

cmd/nvidia-ctk/hook/hook.go

 		},
+		// Handle unrecognized commands when help is requested (e.g., help
+		// unknowncommand)
+		CommandNotFound: func(ctx context.Context, cmd *cli.Command, commandName string) {


These and the Action are now strictly duplicated from nvidia-cdi-hook. I don't think this is required with urfave/v3. Do we want to refactor this?

I did a small refactor, creating a shared function so we don't have duplicated code

elezar · 2025-09-25T06:27:35Z

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

 	}
 }
+
+// NewHookCommand creates a new hook command with common behavior for handling


Nit: This is specifically CDI hooks, so maybe rename NewCDIHookCommand?

elezar · 2025-09-25T06:28:39Z

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

+		// Handle unrecognized commands when help is requested (e.g., help
+		// unknowncommand)
+		CommandNotFound: func(ctx context.Context, cmd *cli.Command, commandName string) {
+			logger.Warningf("Unsupported CDI hook: %v", commandName)


Question: Is there a reason that we don't call IssueUnsupportedHookWarning (or at least reuse some of the logic there)?

elezar · 2025-09-25T06:29:16Z

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

+				if len(args) > 0 && cmd.Command(args[0]) == nil {
+					// This is an unrecognized hook with flags - the default
+					// Action will handle it
+					return nil


Should we not issue a warning here?

elezar · 2025-09-25T06:31:19Z

cmd/nvidia-cdi-hook/main.go

 	opts := options{}

-	// Create the top-level CLI
-	c := cli.Command{


Does it make sense to still construct the Commands and then call a fuction to modify the members that we want instead?

Changes addressed

elezar · 2025-09-25T09:06:05Z

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

+// ConfigureCDIHookCommand takes a base command and adds the common CDI hook
+// behavior for handling unsupported hooks.
+// The base command should have Name, Usage, and Version set as desired.
+func ConfigureCDIHookCommand(base *cli.Command, logger logger.Interface) *cli.Command {


The comment seems to indicate that we should call the functions something like AddUnsupportedHooksChecks but we do add the subcommands at the end, so it's probably just the comment that needs to be udpated for accuracy.

Func doc comment has been updated

elezar · 2025-09-25T09:07:19Z

cmd/nvidia-cdi-hook/main.go

-			return nil
+	}
+	// Set log-level for all subcommands
+	c.Before = func(ctx context.Context, cmd *cli.Command) (context.Context, error) {


Givent that we're only ADDing to the hook in the new function, I would not expect this to be part of the diff.

The diff has been updated

elezar · 2025-09-25T09:08:50Z

Let's update the description to show the output for the various test cases.

elezar · 2025-09-25T12:50:22Z

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

+// ConfigureCDIHookCommand takes a base command and configures it to handle
+// unsupported CDI hooks gracefully.


This is still not accurate.

How about now?

elezar

There are still unaddressed comments.

This change moves the common logic for the nvidia-cdi-hook and nvidia-ctk hook commands to the commands package. Signed-off-by: Evan Lezar <[email protected]>

The initial implementation for skipping unknown hooks did not properly handle hooks with flags. This change ensures that these are properly handled. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez requested review from elezar and Copilot September 22, 2025 09:26

ArangoGutierrez self-assigned this Sep 22, 2025

Copilot AI reviewed Sep 22, 2025

View reviewed changes

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

ArangoGutierrez added this to the v1.18.0 milestone Sep 22, 2025

ArangoGutierrez marked this pull request as ready for review September 22, 2025 09:35

elezar reviewed Sep 24, 2025

View reviewed changes

elezar previously requested changes Sep 24, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from 9bc97f8 to 28c5e29 Compare September 24, 2025 09:18

ArangoGutierrez requested review from elezar and Copilot September 24, 2025 09:18

Copilot AI reviewed Sep 24, 2025

View reviewed changes

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

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

elezar reviewed Sep 24, 2025

View reviewed changes

elezar previously requested changes Sep 24, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from 28c5e29 to 13bbb7d Compare September 24, 2025 12:13

ArangoGutierrez requested a review from elezar September 24, 2025 12:13

elezar reviewed Sep 24, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from 13bbb7d to 4616c6f Compare September 24, 2025 13:09

ArangoGutierrez requested a review from elezar September 24, 2025 13:10

elezar reviewed Sep 25, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from 4616c6f to 57efba1 Compare September 25, 2025 07:13

ArangoGutierrez requested a review from elezar September 25, 2025 07:16

elezar reviewed Sep 25, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from 57efba1 to d7c7174 Compare September 25, 2025 11:22

ArangoGutierrez requested a review from elezar September 25, 2025 11:26

elezar reviewed Sep 25, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from d7c7174 to 5d24958 Compare September 25, 2025 13:22

ArangoGutierrez requested a review from elezar September 25, 2025 13:23

elezar requested changes Sep 25, 2025

View reviewed changes

ArangoGutierrez force-pushed the i1225 branch from 5d24958 to 0a64ad5 Compare September 26, 2025 08:11

ArangoGutierrez requested a review from elezar September 26, 2025 08:12

elezar and others added 2 commits September 26, 2025 13:51

[no-relnote] Refactor construction of hooks

a895b02

This change moves the common logic for the nvidia-cdi-hook and nvidia-ctk hook commands to the commands package. Signed-off-by: Evan Lezar <[email protected]>

Fix handling of unrecognised hooks

c258ca7

The initial implementation for skipping unknown hooks did not properly handle hooks with flags. This change ensures that these are properly handled. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

elezar force-pushed the i1225 branch from 0a64ad5 to c258ca7 Compare September 26, 2025 11:54

elezar changed the title ~~Print a warning on an unrecognised CDI hook~~ Fix handling of unrecognised CDI hooks Sep 26, 2025

elezar approved these changes Sep 26, 2025

View reviewed changes

elezar merged commit fe90b00 into NVIDIA:main Sep 26, 2025
13 checks passed

		// ConfigureCDIHookCommand takes a base command and configures it to handle
		// unsupported CDI hooks gracefully.

Fix handling of unrecognised CDI hooks #1308

Fix handling of unrecognised CDI hooks #1308

Uh oh!

Conversation

ArangoGutierrez commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Cases

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

coveralls commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18008895528

Details

💛 - Coveralls

Uh oh!

ArangoGutierrez commented Sep 22, 2025

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!

elezar left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elezar left a comment

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!

elezar commented Sep 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elezar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ArangoGutierrez commented Sep 22, 2025 •

edited

Loading

coveralls commented Sep 22, 2025 •

edited

Loading