Skip to content

Allow config sources to be specified for containerd #1251

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

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

Allow config sources to be specified for containerd #1251

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
2 changes: 2 additions & 0 deletions cmd/nvidia-ctk-installer/container/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ type Options struct {
SetAsDefault bool
RestartMode string
HostRootMount string

ConfigSources []string
}

// Configure applies the options to the specified config
Expand Down
15 changes: 13 additions & 2 deletions cmd/nvidia-ctk-installer/container/runtime/containerd/containerd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,23 @@ func GetLowlevelRuntimePaths(o *container.Options, co *Options) ([]string, error
}

func getRuntimeConfig(o *container.Options, co *Options) (engine.Interface, error) {
var loaders []toml.Loader
for _, source := range o.ConfigSources {
switch source {
case "file":
loaders = append(loaders, toml.FromFile(o.Config))
case "command":
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: Since command here implies containerd config dump command we may want to make this more expressive. For now I have used the values that we support in the nvidia-ctk runtime configure command:

nvidia-container-toolkit/cmd/nvidia-ctk/runtime/configure/configure.go

Lines 132 to 137 in 7b1b20b

&cli.StringFlag{
Name: "config-source",
Usage: "the source to retrieve the container runtime configuration; one of [command, file]\"",
Destination: &config.configSource,
Value: defaultConfigSource,
},

loaders = append(loaders, containerd.CommandLineSource(o.HostRootMount, o.ExecutablePath))
default:
// TODO: log a warning here.
}
}

return containerd.New(
containerd.WithPath(o.Config),
containerd.WithConfigSource(
toml.LoadFirst(
containerd.CommandLineSource(o.HostRootMount, o.ExecutablePath),
toml.FromFile(o.Config),
loaders...,
),
),
containerd.WithRuntimeType(co.runtimeType),
Expand Down
9 changes: 9 additions & 0 deletions cmd/nvidia-ctk-installer/container/runtime/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ func Flags(opts *Options) []cli.Flag {
Sources: cli.EnvVars("NVIDIA_RUNTIME_SET_AS_DEFAULT", "CONTAINERD_SET_AS_DEFAULT", "DOCKER_SET_AS_DEFAULT"),
Hidden: true,
},
&cli.StringSliceFlag{
Name: "config-source",
Usage: "specify the config sources",
Destination: &opts.ConfigSources,
Sources: cli.EnvVars("RUNTIME_CONFIG_SOURCES"),
Comment on lines +106 to +110
Copy link
Collaborator

Choose a reason for hiding this comment

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

Flag to be called config-source-order (or similar)

specify the order of config sources

As the user can do --config-source command,file or --config-source file,command

same for the ENV VAR RUNTIME_CONFIG_SOURCES_ORDER

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 don't agree that the flag name should be changed. The flag specifies the sources and the ordering is implied by the order in which the sources are specified. Valid values for this argument are currently:

  • ""
  • "file"
  • "command"
  • "command,file" (Default)
  • "file,command"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's expand the Usage to be more explicit, "specify the config sources [file, command]" as nvidia-ctk runtime configure has a flag with the same name but this one is to point to a path. We should provide guidance to the user via Usage with at least the valid options or the default behav of this flag if not set.

},
}

flags = append(flags, containerd.Flags(&opts.containerdOptions)...)
Expand Down Expand Up @@ -137,6 +143,9 @@ func (opts *Options) Validate(logger logger.Interface, c *cli.Command, runtime s
if opts.RestartMode == runtimeSpecificDefault {
opts.RestartMode = containerd.DefaultRestartMode
}
if len(opts.ConfigSources) == 0 {
opts.ConfigSources = []string{"command", "file"}
}
case crio.Name:
if opts.Config == runtimeSpecificDefault {
opts.Config = crio.DefaultConfig
Expand Down