Skip to content

Issue warning on unsupported CDI hook #1000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Issue warning on unsupported CDI hook #1000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions cmd/nvidia-cdi-hook/commands/commands.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,16 @@ func New(logger logger.Interface) []*cli.Command {
cudacompat.NewCommand(logger),
}
}

// IssueUnsupportedHookWarning logs a warning that no hook or an unsupported
// hook has been specified.
// This happens if a subcommand is provided that does not match one of the
// subcommands that has been explicitly specified.
func IssueUnsupportedHookWarning(logger logger.Interface, c *cli.Context) {
args := c.Args().Slice()
if len(args) == 0 {
logger.Warningf("No CDI hook specified")
} else {
logger.Warningf("Unsupported CDI hook: %v", c.Args().Slice()[0])
}
}
12 changes: 12 additions & 0 deletions cmd/nvidia-cdi-hook/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,18 @@ 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 set the default action for the `nvidia-cdi-hook` command to issue a
// warning and exit with no error.
// This means that if an unsupported hook is run, a container will not fail
// to launch. 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.
c.Action = func(ctx *cli.Context) error {
commands.IssueUnsupportedHookWarning(logger, ctx)
return nil
}

// Setup the flags for this command
c.Flags = []cli.Flag{
&cli.BoolFlag{
Expand Down
15 changes: 13 additions & 2 deletions cmd/nvidia-ctk/hook/hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type hookCommand struct {
logger logger.Interface
}

// NewCommand constructs a hook command with the specified logger
// NewCommand constructs CLI subcommand for handling CDI hooks.
func NewCommand(logger logger.Interface) *cli.Command {
c := hookCommand{
logger: logger,
Expand All @@ -37,10 +37,21 @@ func NewCommand(logger logger.Interface) *cli.Command {

// build
func (m hookCommand) build() *cli.Command {
// Create the 'hook' command
// Create the 'hook' subcommand
hook := cli.Command{
Name: "hook",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Usage: "A collection of hooks that may be injected into an OCI spec",
// We set the default action for the `hook` subcommand to issue a
// warning and exit with no error.
// This means that if an unsupported hook is run, a container will not fail
// to launch. 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.
Action: func(ctx *cli.Context) error {
commands.IssueUnsupportedHookWarning(m.logger, ctx)
return nil
},
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down