Codecov Report

Merging #2330 into master will decrease coverage by 0.13%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2330      +/-   ##
==========================================
- Coverage   44.98%   44.85%   -0.14%     
==========================================
  Files          92       93       +1     
  Lines        9340     9368      +28     
==========================================
  Hits         4202     4202              
- Misses       4459     4487      +28     
  Partials      679      679

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a15e7a0...b949697. Read the comment docs.

Hey @crosbymichael, can you explain why support for GPUs is being added to containerd directly?
To give some context, I went down the road of adding GPUs to k8s via the kubelet initially, but then after realizing the complexity, switched to a plugin model in k8s.
How do you plan on generalizing to other devices?

@vishh If you look at the implementation it's not really a deep integration where you can say that it was added containerd directly. It's all about how the spec is generated. I think the existing implementations kinda skew your thinking around adding gpus to a container and make it a little more complex than it has to be. This is not their fault, they had to code around not having options in the Docker API but at the lowest level, its just creating the right dev nodes and mounting the correct drivers into the container.

I still have some work to do on app profiles because that involves creating a custom profile for the container and mounting it in but the implementation as a whole is very simple. You can see that I also added infiniband support using this same logic. All of this is done outside of the daemon, client side for spec generation and we can use a standard runc or runtime that supports device nodes and mounts.

BTW, what complexities did you run into when you tried putting it in the kubelet?

I think the existing implementations kinda skew your thinking around adding gpus to a container and make it a little more complex than it has to be. This is not their fault, they had to code around not having options in the Docker API but at the lowest level, its just creating the right dev nodes and mounting the correct drivers into the container.

Ah, that's not very nice :)
If it was this simple, we wouldn't need our own custom prestart hook for the general use case.
Some points:

• You can't assume a single installation point for our user-level driver libraries, what you have will probably only work for a subset of ubuntu 16.04 users. They can be scattered on the system, and you can have leftovers from previous installs.
• NVIDIA kernel modules might not be loaded, or partially loaded.
• Device files might not exist on the host when runc is called.
• /dev/nvidia-uvm-tools do not exist for older driver versions. More generally, the list of user-space components to mount will vary between versions.
• The device ordering is different between the PCI order (reported by nvidia-smi) and the device minor.
  It's often the opposite: device 0 might be /dev/nvidia7. If you have heterogeneous GPUs, it does matter
  You have to use CUDA+NVML to figure out the proper device ordering.
• We need to fixup our driver stack since it's not aware of namespaces, you might have seen the ModifyDeviceFiles part.
• We hide some procfs entry when doing isolation.
• We need to check compatibility between the image and the runtime. For instance, an application compiled against CUDA 9.1 will require driver 387, on older driver the CUDA initialization will fail. Since this can happen after the (lengthy) CPU-side initialization, we want to prevent this as early as possible.
• Another example is device isolation for OpenGL EGL (I think that's what you are referring to for "app profiles"). It requires a driver >= 390.
• We need to refresh the container's ldcache inside the container, to avoid relying on LD_LIBRARY_PATH (which is often cleared by many programs as we know).

And as I mentioned on the other thread, there will be more and more driver features to support:

• Support for Vulkan. It will probably require writing a custom ICD to the filesystem, and writing an application profile (like EGL).
• Support for GPU sharing with MPS
• Support for nvlink
• Support for vGPU
• Support for fully-containerized drivers, this requires additional logic in the runtime.

I think that InfiniBand is even more complicated. But @3XX0 is the expert here.

In the case of @vishh (GKE) they control the driver install too, so they can have a simplified approach that's tied to the way they install driver. But for enabling all users, we can't make this assumption.

@flx42 I meant the hook approach for everything not the underlying implementation. Sorry if I made it sound like your implementation was overly complex. When we start getting code into containerd or docker, all the configuration that happens inside the hooks shouldn't be needed anymore. I think splitting configuration vs runtime actions is a win for both projects. Again, really sorry that I didn't explain better.

Most of the things you listed here I'm still working on but they are almost all configuration level issues that can be handled before the container is started. The only thing that would HAVE to run inside the container's rootfs is a ldcache refresh. runc already has options for masking proc entries, etc.

When we start getting code into containerd or docker, all the configuration that happens inside the hooks shouldn't be needed anymore.

Except when it can't, like for the libnetwork hook, right?

Most of the things you listed here I'm still working on but they are almost all configuration level issues that can be handled before the container is started.

Did you consider using our CLI too? We have nvidia-container-cli list:

$ nvidia-container-cli list
/dev/nvidiactl
/dev/nvidia-uvm
/dev/nvidia-modeset
/dev/nvidia1
/dev/nvidia0
/usr/bin/nvidia-smi
/usr/bin/nvidia-debugdump
/usr/bin/nvidia-persistenced
/usr/bin/nvidia-cuda-mps-control
/usr/bin/nvidia-cuda-mps-server
/usr/lib/x86_64-linux-gnu/libnvidia-ml.so.390.48
/usr/lib/x86_64-linux-gnu/libnvidia-cfg.so.390.48
/usr/lib/x86_64-linux-gnu/libcuda.so.390.48

You can also use the command-line options to only get a subset:

$ nvidia-container-cli list --device=0
/dev/nvidiactl
/dev/nvidia-uvm
/dev/nvidia-modeset
/dev/nvidia1

$ nvidia-container-cli list --binaries
/usr/bin/nvidia-smi
/usr/bin/nvidia-debugdump
/usr/bin/nvidia-persistenced
/usr/bin/nvidia-cuda-mps-control
/usr/bin/nvidia-cuda-mps-server

@crosbymichael
@flx42 gave a detailed overview of what I meant by "complexity". I did not grok that you are just making configs simpler in this PR and so you can ignore my previous comment.
I agree that designing container support for GPUs to work for traditional systems (VMs or Desktop environments) is more challenging. In the k8s world, I have been suggesting making simplifying assumptions on how nodes are expected to be setup for consuming GPUs to avoid the plethora of possible configurations we have to deal with. That may not be applicable to containerd since it is expected to widely adopted on traditional systems too IIUC.
One more thing that is relevant to this thread is the lack of ability to run exec operations between Create and Start phase of a container to update a container sandbox. My hunch is that exec could be used to handle all the runtime aspects of consuming GPUs and other devices instead of binary OCI hooks.

@vishh ya, perfect idea. If that does not work it's probably a containerd bug and not a runc issue because I remember us specifically allowing create + exec+delete as a workflow. I'll make an issue to follow up on that and get it working.

@mrunalp mentioned that we pivot root as part of create. I was originally surprised to hear that and then later learnt that the Spec does not define any specific behavior around that.
Glad to hear that you can help sort that out.

Oh, that is the case of getting extra binaries in on exec. we could look at exeveat or think of another solution

@crosbymichael sounds good, keep me in the loop. @flx42 what's your take on using exec vs binary hooks?

@vishh After chatting some with @flx42 some tonight, I'm trying to identify what actually needs to be run inside the container vs what can happen on the host. I'll still have to implement some of this but the only thing at the moment that has to be run inside the container is to reload the ldcache (which we could probably do this in a chroot).

The ModifyDeviceFiles can be written in the container's bundle or in one file under the runtime and bind mounted to /proc via the spec. No need to make a tmpfs mount for each container and write a file. The kernel modules can be loaded on the host before the container is created, same with verifying driver versions, this can be done at container create time.

We should not focus too much on the current state of our driver, it might change. It's not impossible that in the future device isolation will not be through cgroups, it's currently an implementation detail. It could be done by echoing your own PID to a special file from our procfs folder, doing a special ioctl call, etc. I can't also guarantee that a single params file in our procfs will be fine for all containers.

I understand your approach, but that would mean it needs to duplicated by each program building an OCI runtime spec. Whereas a prestart hook is already a feature that is portable and needs to be supported by all OCI-compliant runtimes. When support of a feature is such a moving target, I think it does make sense to use prestart hooks, it's easier to upgrade than the full runtime or the container supervisor. Supporting all of these features will add many dependencies to the runtime/supervisor, that's why we believe a hook is a good fit,.
Or maybe we should do a spec hook? :)

I think you will be able to do less things after the create. If you did the pivot_root, you can't do operations that require access to both the runtime and the container namespaces simultaneously. Your privileges will also be dropped, right?
In advanced cases, I think you will need to cross the runtime spec <-> runtime boundary, for instance:

• You need to interact with the host network namespace AND the container network namespace.
• The destination of a mount depends on the content of the filesystem (environment variables, annotations, but also the layout of the rootfs). Or going even further, the content of the filesystem might require you to not do a mount.
• Inside your exec you don't have sufficient privileges (LSMs, userns) to perform the operations that you need, and for reasons not fully under you control you might not always be able to do that on the host.
• If you're spawning VMs + containers inside it, prestart hooks are supposed to be ran inside the VM, there is nothing meaningful you can do on the host side.

It seems possible to not use pivot_root for create:

https://github.com/opencontainers/runc/blob/63e6708c74c1cc46091ec92ea9df5aca4d82e803/libcontainer/rootfs_linux.go#L102-L107

Though i still need to understand how exec will work as an alternate to hook, I guess using a solution which is independent of runtime is more portable and less complex for end-users. hooks approach looks more consistent across different runtimes.

@vishh actually it shouldn't be an issue. We can mount in ldconfig at create time and exec process can run as different users and capability sets. We can run ldconfig on exec and either leave it as a mount or umount it after we are done.

To show what I mean, *specs.Process is what is used to start the exec. Capabilities and LSMs are applied on the process, not the container. https://github.com/opencontainers/runc/pull/1788/files#diff-604f4192deea5adf9c159e5f88f64630R40

Is it fair to say that gpus now require customization to not just the container sandbox, but also the container runtime itself which the existing hooks happen to not differentiate between?

Sorry, I don't get what you are saying here. Could you explain?

@flx42

Sorry, I don't get what you are saying here. Could you explain?

I updated my original comment to be a bit more specific.

@crosbymichael That sounds promising. I'd love to hear what @flx42 thinks about this approach since I'd like to use such a model in k8s land as well if possible.

Thanks @vishh

Ideally, host customization (loading drivers) will be handled out of band from container runtimes

Of course that's possible. We already have those pieces in the hook and in our library/CLI, and they can be disabled. And if everything is already setup correctly, our code path in the library will be no-op.
But again, it's not part of the OCI runtime spec. If all you have is the OCI runtime spec and you don't have the luxury of making simplifying assumptions about the state of the host, you have to put all the logic in the prestart hook.
Today you can set load-kmods = false in /etc/nvidia-container-runtime/config.toml to make sure we won't try to load kernel modules.

runtime isolation customization (namespace, cgroups changes if any) will be handled via runtime plugin

Runtime plugin? You mean a OCI hook? :)

container sandbox customization (ldconfig, CUDA version check, etc.) will be handled via exec plugins.

Ok for ldconfig, too late for the CUDA version check.

@vishh the other option we have been discussing is using the low level nvidia-containerd-cli. I have a branch here if you want to check it out:

https://github.com/containerd/containerd/compare/master...crosbymichael:nvidia-no-hook?expand=1

The general idea is that it uses the lowest level libnvidia-container lib and cli, as a hook still, but replaces the current container hook with a generic one.

I agree with everything you said in your updated comment, I wish we could make that work that way and that was my original goal but I'm not sure anymore. We need more support.

I updated this PR to point to my branch with the nvidia-container-cli took and generic hook within containerd to support having generic CLI binaries as OCI hooks. It templates out information for args and env so that we can keep the nvidia support client side configuration only and not require any daemon level code.

@containerd/containerd-maintainers please take another look at this. The implementation changed from the first time this PR was submitted.