Fetch current container runtime config #686
Conversation
tariq1890
force-pushed
the
get-config-from-cmdline
branch
3 times, most recently
from
1a5292f
to
56b7927
Compare
tariq1890
force-pushed
the
get-config-from-cmdline
branch
8 times, most recently
from
310ee8a
to
adf9026
Compare
tools/container/toolkit/toolkit.go
Outdated
| toolkitRuntimeList := getNvidiaContainerRuntimeList(cfg, opts.ContainerRuntimeRuntimes.Value()) | ||
| if len(toolkitRuntimeList) > 0 { | ||
| configValues["nvidia-container-runtime.runtimes"] = toolkitRuntimeList | ||
| } |
There was a problem hiding this comment.
This should no longer be required.
We will ONLY set the binaries from the runtimes. Note that if opts.ContainerRuntimeRuntimes is set we will overwrite the value we set here as part of processing the "options" config options.
There was a problem hiding this comment.
We will ONLY set the binaries from the runtimes.
I don't think we can do that. If you look at the default containerd config toml, the BinaryName fields are empty, meaning that the PATH-resolvable low-level-runtime binary paths will be used. So we still need the default list ["docker-runc", "runc", "crun"]
There was a problem hiding this comment.
Could we implement it so that if the slice is empty we don't update the config. Alternatively see the comment below.
There was a problem hiding this comment.
Thanks @tariq1890.
I think this has come together very nicely. I have some additional comments.
|
Thanks for the work on this @tariq1890. One question that I had was whether we should update how we apply the modified config. At present we're going to generate a file that includes the full config. This may not be desirable when we are using drop-in files in crio, for example. I don't think it's a blocker for this iteration, but it would be good to put some thought into how the system will behave under these conditions. |
tariq1890
force-pushed
the
get-config-from-cmdline
branch
3 times, most recently
from
a0ddda5
to
6266ef3
Compare
Agreed, one way is to modify the command line config source to only return the |
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com> add default runtime binary path to runtimes field of toolkit config toml Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com> [no-relnote] Get low-level runtimes consistently We ensure that we use the same low-level runtimes regardless of the runtime engine being configured. This ensures consistent behaviour. Signed-off-by: Evan Lezar <elezar@nvidia.com> Co-authored-by: Evan Lezar <elezar@nvidia.com> address review comment Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
tariq1890
force-pushed
the
get-config-from-cmdline
branch
from
3e6c2b7
to
f477dc0
Compare
Summary of changes made in this PR:
In CRI-O and Containerd config engines, retrieve the container runtime config commands via the following commands:
i.
crio status configii.
containerd config dumpWhen configuring CRI-O runtime, we prioritise the runtime designated as the
default_runtimein the config when setting up thenvidiaruntime handler as opposed to favouring theruncruntime handler at all times. This is needed as vanilla cri-o packages havecrunas the default low-level-runtime. Live-swapping the low-level runtime causes the running containers in a cluster to breakAdd the full path of the low-level runtime to the
nvidia-container-runtime.runtimesconfig value, so thatnvidia-container-runtimebinds to a low-level runtime binary that cannot be resolved in thePATH.