Add DeviceRequests to HostConfig to support NVIDIA GPUs #38828

tiborvass · 2019-03-05T04:10:06Z

This patch hard-codes support for NVIDIA GPUs.
In a future patch it should move out into its own Device Plugin.

Signed-off-by: Tibor Vass [email protected]

Notes

I tried to keep the API generic enough for devices other than GPU for the future.

In addition to "options", there's this generic notion of "capabilities" that any device can advertise a set of, and then there's a matching happening when requesting devices. This should help with @thaJeztah and @tonistiigi's concerns about mixing "constraints" and "settings" (aka "options"). This is going to be even more important for orchestration and secure setups like in buildkit, where you'll have a separation between device requesters and providers.

For instance, any GPU vendor should add the "gpu" device capability to its docker device driver. They can also advertise other caps which can be used for matching the driver (and in the future, the node in a cluster).

Currently, the nvidia driver doesn't do anything with "options", but in the future it could decide to limit how much GPU memory should be used for instance. It's all the settings that would not be used for scheduling. Device capabilities include NVIDIA capabilities (compute, utility, etc.).

I'm happy to bikeshed on names, but would love to get review on the API itself.

cc @RenaudWasTaken @cpuguy83 @crosbymichael

thaJeztah · 2019-03-05T14:31:20Z

Some linting failures;

04:24:38 pkg/capabilities/caps.go:1::warning: file is not gofmted with -s (gofmt)
04:24:38 pkg/capabilities/caps.go:1::warning: file is not goimported (goimports)
04:24:38 daemon/nvidia_linux.go:33:29:warning: unnecessary conversion (unconvert)

cpuguy83 · 2019-03-05T15:22:56Z

Why are we doing this instead of the PR from Nvidia?

thaJeztah · 2019-03-05T15:53:03Z

linking the other PR's; #37434 and #37504

tiborvass · 2019-03-05T19:25:06Z

@cpuguy83 Because there's nothing specific about GPUs, it's just devices with prestart hooks. It's akin to Kubernetes device plugins. On the CLI however, it's --gpu.

daemon/nvidia_linux.go

pkg/capabilities/caps.go

RenaudWasTaken

A single small comment otherwise ship it!

daemon/nvidia_linux.go

api/swagger.yaml

thaJeztah · 2019-03-12T22:13:26Z

api/types/container/host_config.go

+	Driver       string            // Name of device driver
+	Count        int               // Number of devices to request (-1 = All)
+	DeviceIDs    []string          // List of device IDs as recognizable by the device driver
+	Capabilities [][]string        // An OR list of AND lists of device capabilities (e.g. "gpu")


We were discussing Capabilities as a name for this (as it could be confused for Capabilities on the container itself (i.e. Linux capabilities)), but I can't come up with good alternatives; perhaps Features, but not sure if that's a good match

I understand it but on the other hand, it's literally a list of what the device is capable of doing, what capabilities it provides. In this case it provides "gpu" capability, as well as nvidia-specific capabilities like "compute", etc.

The only other names I can think of is "requirements" or "constraints", but I'm unsure.

It only matches if all of these are matched, correct? "constraints" could work, but possibly too generic? idk. Naming is really hard on this one

thaJeztah

design LGTM

api/swagger.yaml

codecov · 2019-03-13T07:39:57Z

Codecov Report

❗ No coverage uploaded for pull request base (master@36d2c8b). Click here to learn what that means.
The diff coverage is 22.22%.

@@            Coverage Diff            @@
##             master   #38828   +/-   ##
=========================================
  Coverage          ?   36.41%           
=========================================
  Files             ?      617           
  Lines             ?    45950           
  Branches          ?        0           
=========================================
  Hits              ?    16732           
  Misses            ?    26929           
  Partials          ?     2289

tiborvass · 2019-03-13T20:09:33Z

Updated

thaJeztah

Left some comments inline, and saw that containerd/containerd#3093 was merged (so we can use the exported list)

Also could you add code to ignore the new field on older API versions on container-create?

moby/api/server/router/container/container_routes.go

Lines 468 to 485 in ca0b64e

    
           if hostConfig != nil && versions.LessThan(version, "1.40") { 
        
           	// Ignore BindOptions.NonRecursive because it was added in API 1.40. 
        
           	for _, m := range hostConfig.Mounts { 
        
           		if bo := m.BindOptions; bo != nil { 
        
           			bo.NonRecursive = false 
        
           		} 
        
           	} 
        
           	// Ignore KernelMemoryTCP because it was added in API 1.40. 
        
           	hostConfig.KernelMemoryTCP = 0 
        
           	// Ignore Capabilities because it was added in API 1.40. 
        
           	hostConfig.Capabilities = nil 
        
           	// Older clients (API < 1.40) expects the default to be shareable, make them happy 
        
           	if hostConfig.IpcMode.IsEmpty() { 
        
           		hostConfig.IpcMode = container.IpcMode("shareable") 
        
           	} 
        
           }

pkg/capabilities/caps_test.go

pkg/capabilities/caps.go

daemon/devices_linux.go

thaJeztah · 2019-03-14T22:36:47Z

daemon/devices_linux.go

+func (daemon *Daemon) handleDevice(req container.DeviceRequest, spec *specs.Spec) error {
+	if req.Driver == "" {
+		for _, dd := range deviceDrivers {
+			if selected := dd.capset.Match(req.Capabilities); selected != nil {


One thing I'm wondering: here, we match capabilities against the driver. So if a machine has (e.g.) two GPUs, and one of them supports "capA" and one of them "capB", then the driver would register itself with all of those (so driver says: "I provide capA and capB") correct?

This could result in a situation where none of the GPUs support the requested list of capabilities, i.e.;

Request GPU-A GPU-B Driver Driver Match GPU Match

"capA,capB" "capA, capC" "capB, capC" "capA,capB,capC" ✅ ❌

What would happen in that case? (i.e., conversion to OCI succeeds, hook is registered, but no GPU is found)? Will a proper error be produced?

I could make it an OR list of ANDs as well instead of a map.

Perhaps we should if this is a concern, so in that case the driver would report itself as;

{ "capabilities": [ ["capA", "capB"], ["capB", "capC"] ] }

Could even decide to make it just return a list of capabilities for each GPU (then we can even determine the number of GPUs available);

{ "capabilities": [ ["capA", "capB"], ["capA", "capB"], ["capA", "capB"], ["capA", "capB"], ["capB", "capC"] ] }

But perhaps that breaks the abstraction

I suggest we punt on the problem since the problem is extremely unlikely to happen at this time and the structure is that of the device driver so it's internal, we can change it. The API needs to be locked down.

daemon/devices_linux.go

api/swagger.yaml

thaJeztah · 2019-03-14T22:55:28Z

api/swagger.yaml

+          items:
+            type: "string"
+        example:
+          # gpu AND nvidia AND compute


Do we need an example for OR here?

Suggested change

# gpu AND nvidia AND compute

# gpu AND nvidia AND compute, OR gpu AND intel

- ["gpu", "nvidia", "compute"]

- ["gpu", "intel"]

No it's fine, the reason I put it there is so that we can support it in the future without breaking the API.

If we don't want to support OR yet; we should error out if len(capabilities) > 1

What I meant is that it is supported, but not from the CLI.

api/swagger.yaml

tiborvass · 2019-03-15T17:14:49Z

@thaJeztah thanks for your review, I updated again.

thaJeztah

LGTM, thanks!

thaJeztah · 2019-03-16T16:44:52Z

ping @cpuguy83 @kolyshkin ptal

thaJeztah · 2019-03-16T16:46:16Z

docs/api/version-history.md

@@ -49,6 +49,8 @@ keywords: "API, Docker, rcli, REST, documentation"
 * `GET /info` now returns information about `DataPathPort` that is currently used in swarm
 * `GET /info` now returns `PidsLimit` boolean to indicate if the host kernel has
  PID limit support enabled.
+* `GET /containers/create` now accepts `DeviceRequests` as part of `HostConfig`.


Oh, erm,

Suggested change

* `GET /containers/create` now accepts `DeviceRequests` as part of `HostConfig`.

* `POST /containers/create` now accepts `DeviceRequests` as part of `HostConfig`.

* `GET /containers/{id}/json` now returns `DeviceRequests` as part of `HostConfig`.

thaJeztah · 2019-03-18T14:30:08Z

@tiborvass vendoring is failing;

22:23:22 The result of vndr differs
22:23:22 
22:23:22  M vendor/github.com/containerd/containerd/contrib/nvidia/nvidia.go
22:23:22 
22:23:22 Please vendor your package with github.com/LK4D4/vndr.

This patch hard-codes support for NVIDIA GPUs. In a future patch it should move out into its own Device Plugin. Signed-off-by: Tibor Vass <[email protected]>

vdemeester

LGTM 🐯
But @tiborvass, we need to create issues related to the TODO's in there (to track future work)

tiborvass · 2019-03-19T18:32:36Z

I'm merging since I see those windows errors are unrelated, they are also on other PRs.

GordonTheTurtle added the status/0-triage label Mar 5, 2019

tiborvass mentioned this pull request Mar 5, 2019

container: --gpus support docker/cli#1714

Merged

tiborvass force-pushed the nvidia-gpu branch from 817e499 to 115fa27 Compare March 5, 2019 04:21

thaJeztah added status/1-design-review impact/api impact/changelog and removed status/0-triage labels Mar 5, 2019

wk8 reviewed Mar 7, 2019

View reviewed changes

daemon/nvidia_linux.go Outdated Show resolved Hide resolved

daemon/nvidia_linux.go Outdated Show resolved Hide resolved

daemon/nvidia_linux.go Show resolved Hide resolved

pkg/capabilities/caps.go Show resolved Hide resolved

RenaudWasTaken reviewed Mar 12, 2019

View reviewed changes

daemon/nvidia_linux.go Outdated Show resolved Hide resolved

tiborvass force-pushed the nvidia-gpu branch from 115fa27 to 89aa7c7 Compare March 12, 2019 22:08

thaJeztah reviewed Mar 12, 2019

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Mar 12, 2019

tiborvass force-pushed the nvidia-gpu branch from 89aa7c7 to 75077af Compare March 12, 2019 23:20

thaJeztah requested changes Mar 12, 2019

View reviewed changes

api/swagger.yaml Outdated Show resolved Hide resolved

api/swagger.yaml Outdated Show resolved Hide resolved

tiborvass force-pushed the nvidia-gpu branch 2 times, most recently from 6904937 to 0e85958 Compare March 13, 2019 05:16

tonistiigi mentioned this pull request Mar 14, 2019

[Feature request] Support --runtime option moby/buildkit#842

Closed

thaJeztah requested changes Mar 14, 2019

View reviewed changes

tiborvass force-pushed the nvidia-gpu branch 2 times, most recently from 7100ebf to accc53a Compare March 15, 2019 17:13

thaJeztah approved these changes Mar 16, 2019

View reviewed changes

thaJeztah reviewed Mar 16, 2019

View reviewed changes

tiborvass force-pushed the nvidia-gpu branch from accc53a to cb63792 Compare March 16, 2019 22:09

thaJeztah added the status/needs-vendoring label Mar 18, 2019

Add DeviceRequests to HostConfig to support NVIDIA GPUs

8f936ae

This patch hard-codes support for NVIDIA GPUs. In a future patch it should move out into its own Device Plugin. Signed-off-by: Tibor Vass <[email protected]>

tiborvass force-pushed the nvidia-gpu branch from cb63792 to 8f936ae Compare March 18, 2019 17:20

vdemeester approved these changes Mar 19, 2019

View reviewed changes

thaJeztah added rebuild/* and removed status/needs-vendoring labels Mar 19, 2019

GordonTheTurtle removed the rebuild/* label Mar 19, 2019

tiborvass merged commit 07bb45e into moby:master Mar 19, 2019

tiborvass mentioned this pull request Mar 21, 2019

dockerd gpu support #37504

Closed

tiborvass deleted the nvidia-gpu branch July 17, 2019 00:34

jamesdbrock mentioned this pull request Aug 27, 2019

Use runtime nvidia jupyterhub/dockerspawner#244

Closed

robertgzr mentioned this pull request Oct 29, 2019

Catch up with docker 19.03 CE balena-os/balena-engine#188

Closed

5 tasks

jedevc mentioned this pull request Oct 13, 2022

Support RUN --gpus ... or equivalent. moby/buildkit#1436

Open

thaJeztah mentioned this pull request Jun 30, 2024

pkg/capabilities move to daemon/internal #48101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DeviceRequests to HostConfig to support NVIDIA GPUs #38828

Add DeviceRequests to HostConfig to support NVIDIA GPUs #38828

tiborvass commented Mar 5, 2019 •

edited

Loading

thaJeztah commented Mar 5, 2019

cpuguy83 commented Mar 5, 2019

thaJeztah commented Mar 5, 2019

tiborvass commented Mar 5, 2019

RenaudWasTaken left a comment

thaJeztah Mar 12, 2019

tiborvass Mar 12, 2019

tiborvass Mar 13, 2019

thaJeztah Mar 13, 2019

thaJeztah left a comment

codecov bot commented Mar 13, 2019 •

edited

Loading

tiborvass commented Mar 13, 2019

thaJeztah left a comment

thaJeztah Mar 14, 2019

tiborvass Mar 15, 2019

thaJeztah Mar 15, 2019 •

edited

Loading

tiborvass Mar 15, 2019

thaJeztah Mar 14, 2019

tiborvass Mar 14, 2019

thaJeztah Mar 14, 2019

tiborvass Mar 15, 2019

tiborvass commented Mar 15, 2019

thaJeztah left a comment

thaJeztah commented Mar 16, 2019

thaJeztah Mar 16, 2019

tiborvass Mar 16, 2019

thaJeztah commented Mar 18, 2019

vdemeester left a comment

tiborvass commented Mar 19, 2019

	if hostConfig != nil && versions.LessThan(version, "1.40") {
	// Ignore BindOptions.NonRecursive because it was added in API 1.40.
	for _, m := range hostConfig.Mounts {
	if bo := m.BindOptions; bo != nil {
	bo.NonRecursive = false
	}
	}
	// Ignore KernelMemoryTCP because it was added in API 1.40.
	hostConfig.KernelMemoryTCP = 0

	// Ignore Capabilities because it was added in API 1.40.
	hostConfig.Capabilities = nil

	// Older clients (API < 1.40) expects the default to be shareable, make them happy
	if hostConfig.IpcMode.IsEmpty() {
	hostConfig.IpcMode = container.IpcMode("shareable")
	}
	}

-          # gpu AND nvidia AND compute
+          # gpu AND nvidia AND compute, OR gpu AND intel
+          - ["gpu", "nvidia", "compute"]
+          - ["gpu", "intel"]

	* `GET /containers/create` now accepts `DeviceRequests` as part of `HostConfig`.
	* `POST /containers/create` now accepts `DeviceRequests` as part of `HostConfig`.
	* `GET /containers/{id}/json` now returns `DeviceRequests` as part of `HostConfig`.

Add DeviceRequests to HostConfig to support NVIDIA GPUs #38828

Add DeviceRequests to HostConfig to support NVIDIA GPUs #38828

Conversation

tiborvass commented Mar 5, 2019 • edited Loading

thaJeztah commented Mar 5, 2019

cpuguy83 commented Mar 5, 2019

thaJeztah commented Mar 5, 2019

tiborvass commented Mar 5, 2019

RenaudWasTaken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 13, 2019 • edited Loading

Codecov Report

tiborvass commented Mar 13, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Mar 15, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah commented Mar 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Mar 18, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tiborvass commented Mar 19, 2019

tiborvass commented Mar 5, 2019 •

edited

Loading

codecov bot commented Mar 13, 2019 •

edited

Loading

thaJeztah Mar 15, 2019 •

edited

Loading