There's already some work (in swarmkit) around "GenericResources".
I wonder if that can, or should, be brought over into the runtime side for this.

https://godoc.org/github.com/docker/docker/api/types/swarm#GenericResource

It's a bit more abstract because it's designed to be pretty loose as resources are defined by admins for a given node and the scheduler matches the resources defined on the node with resources defined in the service.... so maybe not necessarily the best fit.

Yes, the GenericResource support was added in swarmkit by my colleague @RenaudWasTaken. As you said this existing API seems to be a better fit for scheduling/matching purposes, and we'll lose the per-vendor separation that I think is useful for API users; but on the other hand this type might be reused for other purposes in the future too.

But those are not major problems, we could work with the existing GenericResource type, if we also attach option to each resource.

So the fundamental question here is:

• Do we want a GPU resource type? Or do we want a more generic API similar to Swarm?

I'm fine either ways.

I think a GPU type works fine and is much simpler to deal with.

Updated my PR, we now have the following:

package container

type Resources struct {
        GPUsByVendor map[gpu.Vendor]*gpu.Resources
        [...]
}

package gpu

type Vendor string

type Resources struct {
        GPUs    []GPU
        Options map[string]string
}

type GPU struct {
        ID string
}

I don't think we need more than that today. If we agree on the API, I can start to flesh out the next step: integration in dockerd by transforming these GPU resources into OCI annotations, then calling into the new containerd API.

I have a concern with this approach:

type Resource {
    Type string // e.g. 'nvidia'
    ID string // GPU UUID... whatever the vendor defines
    Options map[string]string
}

Some options only make sense if they are applied to all GPUs from the same vendor. For instance, for us, the following would be non-sensical:

{ "nvidia", "0", {"capabilities": "compute"}}, { "nvidia", "1", {"capabilities": "video"}}

The capabilities option can't be different for each GPU. While we can enforce that from the CLI, we can't at the API-level. Of course, it's always possible to reject such a construct when the vendor-specific code iterates through its GPUs.

My API above assumes that other vendors will be in the same situation, so I wanted to be able to enforce that at the API level. With this knowledge, if you agree to put this burden on the vendor implementation, then your approach is fine.

@flx42 Thanks for the feedback! So when you say "all" of the GPU's, does that mean literally all GPU's on the system or all the selected GPUs?

Do we know anyone from AMD that would be able to provide input here as well?

So when you say "all" of the GPU's, does that mean literally all GPU's on the system or all the selected GPUs?

All the selected GPUs for the nvidia vendor. Or in other words, this option applies at the container level.

Maybe @kad can chime in on the Intel side for OpenCL, maybe they could have a similar option for selecting/modifying the OpenCL ICD.

@flx42 It seems like your structs work well (but still would prefer a slice over a map in hostconfig, and leave it up to the implementation to validate). Perhaps we can change Resource to GPUSet?

Just to confirm, to avoid re-triggering the CI with a push, are you OK with the following?

package container

type GPUSet struct {
        Vendor string // or "Type"
        GPUs    []GPU
        Options map[string]string
}

type GPU struct {
        ID string
}

type Resources struct {
        GPUs []*GPUSet
        [...]
}

Is Resources needed here? I would think we'd have []GPUSet in HostConfig.

Well the existing Resources struct is embedded in HostConfig:

I think it makes sense to consider a GPU as a resource since that's where CPU, char/block devices, memory are.

The Resources struct in #37434 (comment) is the existing one, in case it was not clear, sorry :)

Ah I see, yeah that's perfect.

Made the changes, also added Options to struct GPU, even if the CLI proposal won't probably support per-GPU options initially.

Does anyone disagree with the current approach?

Assuming we are OK with this design, I will go ahead with the corresponding implementation in the CLI (`--gpus) and in dockerd (for calling into the containerd features).
We can merge once we have everything.

Codecov Report

Merging #37434 into master will increase coverage by 0.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #37434      +/-   ##
==========================================
+ Coverage   35.31%   35.78%   +0.46%     
==========================================
  Files         616      617       +1     
  Lines       48851    51403    +2552     
==========================================
+ Hits        17254    18396    +1142     
- Misses      29225    30422    +1197     
- Partials     2372     2585     +213

@yongtang looks that this one passes tests. PTAL

/cc @cpuguy83

Hello!

I'll be taking over @flx42's PRs.

@cpuguy83 do you have any problems left with the current API? If not can we go ahead and merge this?
Thanks!

Mainly concerned about other GPU vendors chiming in, but no one has in all this time... moving to code review.