Deviceplugin jiayingz #51209

jiayingz · 2017-08-23T17:25:02Z

What this PR does / why we need it:
This PR implements the kubelet Device Plugin Manager.
It includes four commits implemented by @RenaudWasTaken and a commit that supports allocation.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
Design document: kubernetes/community#695
PR tracking: kubernetes/enhancements#368

Special notes for your reviewer:

Release note:
Extending Kubelet to support device plugin

k8s-ci-robot · 2017-08-23T17:25:10Z

Hi @jiayingz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

RenaudWasTaken · 2017-08-23T18:00:09Z

pkg/kubelet/cm/container_manager_linux.go

+				if d, ok := devsMap[dev.ContainerPath]; ok {
+					glog.V(3).Infof("skip existing device %s %s", dev.ContainerPath, dev.HostPath)
+					if (d != dev.HostPath) {
+						glog.Errorf("Container device %s has conflicting mapping host devices: %s and %s",


Should we fail the pod / return an error at this point ?

Same question for Mounts and Envs

My feeling is this kind of misconfiguration may not be fatal. E.g., say the same device is accessible through multiple paths and either is fine. Since we log error here, I think we can make this information accessible through npm for debugging purpose.

RenaudWasTaken · 2017-08-23T18:27:47Z

pkg/kubelet/cm/device_plugin_handler.go

 }

 // NewDevicePluginHandler create a DevicePluginHandler
-func NewDevicePluginHandler() (*DevicePluginHandlerImpl, error) {
+func NewDevicePluginHandler(enabled bool, updateCapacity updateCapacityCallback) (*DevicePluginHandlerImpl, error) {
+	if !enabled {


I wonder if should use the same pattern as for the GPU manager. As in:
type DevicePluginHandler interface
type DevicePluginHandlerImpl struct
type MockDevicePluginHandler struct

And when calling NewDevicePluginHandler instead of passing a bool:

if enabled { cm.handler = NewDevicePluginHandlerImpl() } else { cm.handler = NewMockDevicePluginHandler() }

That would avoid the if at the start of every methods and is a lot cleaner :)

Good suggestion. Done.

RenaudWasTaken · 2017-08-23T18:30:30Z

pkg/kubelet/cm/device_plugin_handler.go


-	mgr, err := deviceplugin.NewManagerImpl(pluginapi.DevicePluginPath,
-		func(r string, a, u, d []*pluginapi.Device) {})
+	deviceManagerMonitorCallback := func(resourceName string, added, updated, deleted []*pluginapi.Device) {


Maybe this would fit more as a method of the handler ?
You would then be able to call NewManagerImpl:

deviceplugin.NewManagerImpl(pluginapi.KubeletSocket, handler.deviceManagerMonitorCallback)

I think this function is to be called by manager only. In that sense, there doesn't seem to be much value to store it in deviceplugin.

RenaudWasTaken · 2017-08-23T19:00:01Z

pkg/kubelet/kubelet_node_status.go

@@ -572,6 +571,19 @@ func (kl *Kubelet) setNodeStatusMachineInfo(node *v1.Node) {
 				}
 			}
 		}
+
+		if utilfeature.DefaultFeatureGate.Enabled(features.DevicePlugins) {


The mock structure would also remove this if

It is probably safe to skip this feature checking anyway, but I am following the current model as LocalStorageCapacityIsolation.

@jiayingz Local storage does not have a stub concept. We do not need this check here as @RenaudWasTaken pointed out.

removed now.

cmluciano · 2017-08-23T21:06:06Z

/ok-to-test

vikaschoudhary16 · 2017-08-24T07:21:48Z

pkg/kubelet/cm/container_manager_linux.go

+		}
+	}
+
+	glog.Infof("Creating device plugin handler: %t", devicePluginEnabled)


Should log level be used?

This is a one-time log, so probably ok to stay as info.

vikaschoudhary16 · 2017-08-24T07:29:23Z

pkg/kubelet/cm/device_plugin_handler.go

+	handler := &DevicePluginHandlerImpl{
+		allDevices:          make(map[string]sets.String),
+		allocated:           devicesInUse(),
+		updateCapacity:	     updateCapacity,


Nit: s/updateCapacity/updateCapacityFunc or something similar to hint a callback function variable name?

vikaschoudhary16 · 2017-08-24T07:51:50Z

pkg/kubelet/cm/container_manager_linux.go

@@ -544,6 +572,13 @@ func (cm *containerManagerImpl) Start(node *v1.Node, activePods ActivePodsFunc)
 		}
 		close(stopChan)
 	}, time.Second, stopChan)
+
+	// Starts device plugin manager.
+/*


Should be safe to remove this part?

vikaschoudhary16 · 2017-08-24T07:55:36Z

pkg/kubelet/cm/container_manager_linux.go

+func (cm *containerManagerImpl) GetResources(pod *v1.Pod, container *v1.Container, activePods []*v1.Pod) (*kubecontainer.RunContainerOptions, error) {
+	opts := &kubecontainer.RunContainerOptions{}
+	// Gets devices, mounts, and envs from device plugin handler.
+	glog.V(3).Infof("Calling device_plugin AllocateDevices")


Nit: s/device_plugin/device_plugin handler ?

vikaschoudhary16 · 2017-08-24T08:17:39Z

pkg/kubelet/cm/device_plugin_handler.go

+	}
+}
+
+// updateAllocatedGPUs updates the list of GPUs in use.


nit: updateAllocatedGPUs/updateAllocatedDevices

vikaschoudhary16 · 2017-08-24T08:21:56Z

pkg/kubelet/cm/device_plugin_handler.go

+	devicePluginManager *deviceplugin.ManagerImpl
+	// allDevices and allocated are keyed by resource_name.
+	allDevices map[string]sets.String
+	allocated  map[string]*podDevices


nit: s/allocated/allocatedDevices

vikaschoudhary16 · 2017-08-24T08:27:21Z

pkg/kubelet/deviceplugin/manager.go

+func (m *ManagerImpl) Devices() map[string][]*pluginapi.Device {
+	glog.V(2).Infof("Devices called")
+
+	m.mutex.Lock()


we are already using locks in device plugin handler, then what race condition this second level of locking is avoiding?

I'm not sure this should be read from a top down perspective @vikaschoudhary16 but instead bottom up especially for locks :)

Anyways to answer your question the lock here is to prevent Endpoints from changing.

This part is from Renaud's commit but I think in general, each layer would have its own lock to protect its internal data structure. This prevents future breakage when we move things around.

vikaschoudhary16 · 2017-08-24T08:49:56Z

pkg/kubelet/cm/device_plugin_handler.go

+		available := h.allDevices[key].Difference(devicesInUse)
+		glog.V(2).Infof("devices available: %v", available.List())
+		if int(available.Len()) < needed {
+			return nil, fmt.Errorf("requested number of devices unavailable. Requested: %d, Available: %d", v, available.Len())


Should it be needed instead of v?

vikaschoudhary16 · 2017-08-24T09:14:57Z

pkg/kubelet/cm/container_manager_linux.go

+	for _, resp := range allocResps {
+		for _, devRuntime := range resp.Spec {
+			for _, dev := range devRuntime.Devices {
+				if d, ok := devsMap[dev.ContainerPath]; ok {


Wondering how can this if condition get pass ever. At L#618, devMaps is being initialized to an empty map, how can devsMap[dev.ContainerPath] be non-empty in any case?

Good catch. Fixed. Note to myself, should add a unit test to cover this :).

vikaschoudhary16 · 2017-08-24T09:32:27Z

pkg/kubelet/deviceplugin/utils.go

+
+// Returns whether the ResourceName points to a device plugin name.
+func IsDeviceName(k v1.ResourceName) bool {
+       return v1helper.IsExtendedResourceName(k)


IsExtendedResourceName definition is missing.

It is a helper function added by Connor's PR. Should already be merged.

jiayingz · 2017-08-25T22:42:33Z

/assign @vishh

FYI, this PR is ready for review.

vikaschoudhary16 · 2017-08-28T05:27:44Z

pkg/kubelet/cm/device_plugin_handler.go

+			return nil, fmt.Errorf("requested number of devices unavailable. Requested: %d, Available: %d", needed, available.Len())
+		}
+		allocated := available.UnsortedList()[:needed]
+		resp, err := h.devicePluginManager.Allocate(key, append(devices.UnsortedList(), allocated...))


@RenaudWasTaken @jiayingz one suggestion, I think we should invoke h.devicePluginManager.Allocate outside of mutex lock because plugin manager is already using locks. Mutex of device plugin handler should be used only to access handler's internal data structures like h.allocatedDevices for example. This looks to me performance efficient than existing implementation. Thoughts?

+1. A remote RPC with a lock is probably not a good idea.

vikaschoudhary16 · 2017-08-28T05:45:39Z

pkg/kubelet/deviceplugin/endpoint.go

+
+}
+
+func (e *endpoint) allocate(devs []string) (*pluginapi.AllocateResponse, error) {


@RenaudWasTaken @jiayingz Pod/container details should also be passed along with device ids to the plugin. One use case for this requirement is special NIC resources such as Solarflare and Infiniband. With current API, it is possible to mount device files and libs inside container but the corresponding network interface will be available to use inside container only if --net=host is used. This is kinda very strict limitation.
Instead, if pod details are also passed to the device plugin, device plugin can pass the allocated interface from the host net namespace to container net namespace and thus making it possible to use such pass through network interfaces inside non net=host containers as well.
NICs are one such use case, in future there could be many more. We should start with a wider scope and pass pod details as well to the device plugins and depending on requirement, plugins may or may not use these details. Thoughts?
cc @vishh @jeremyeder @sjenning @cmluciano

@vikaschoudhary16 the unofficial, unvetted plan is to have device plugins drop a CNI config along with necessary CNI utility binaries, and have device plugins return additional interface via CNI constructs. That way the device plugin API can be invoked during pod creation rather than during container creation.
In the future, if the networking SIG ends up having a similar RPC based CNI API, then the workflow will become simpler.

The unofficial plan sounds good to me on the surface. I think @3XX0 and I determined that an CNI plugin for Infiniband would probably be necessary.

Agree. I think the discussion on CNI integration will probably wait after 1.8.

vikaschoudhary16 · 2017-08-28T12:33:24Z

pkg/kubelet/cm/device_plugin_handler.go

+	return handler, nil
+}
+
+func (h *DevicePluginHandlerImpl) Start() error {


I could not find from where this is being invoked. Should it be invoked from within NewDevicePluginHandlerImpl()?

No. Object creation is different from logical initialization of an object. The latter may have transitive dependencies on state of other objects and hence requires careful co-ordination initialization.

vikaschoudhary16 · 2017-08-28T12:34:39Z

pkg/kubelet/cm/device_plugin_handler.go

+
+// NewDevicePluginHandler create a DevicePluginHandler
+func NewDevicePluginHandlerImpl(updateCapacityFunc updateCapacityCallback) (*DevicePluginHandlerImpl, error) {
+	glog.V(2).Infof("Starting Device Plugin Handler")


This function is not invoking Start of device plugin handler.

Calling to Start now moves to containerManagerImpl.Start().

Yeah, the log should be updated to say that it's "creating" device plugin handler or maybe move the log to Start() method.

jiayingz · 2017-09-01T18:12:41Z

Lost lgtm after rebase. @dchen1107 and @vishh could you help lgtm this PR again? Thanks!

vishh · 2017-09-01T21:49:51Z

/lgtm

k8s-github-robot · 2017-09-01T21:50:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, jiayingz, vishh

Associated issue: 695

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubelet/OWNERS~~ [dchen1107,vishh]
~~pkg/OWNERS~~ [dchen1107]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-02T18:12:08Z

Automatic merge from submit-queue (batch tested with PRs 51590, 48217, 51209, 51575, 48627)

resouer · 2017-09-28T14:22:04Z

pkg/kubelet/deviceplugin/device_plugin_stub.go

+		}
+		time.Sleep(1 * time.Second)
+	}
+	log.Println("Starting to serve on", m.socket)


@jiayingz Why use log.Println() instead of glog? Is this a separate binary?

Hi @resouer this is fixed in a follow up PR.
device_plugin_stub.go is invoked from tests only though :)

Thanks for the review!

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 23, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2017

k8s-github-robot assigned resouer and sjpotter Aug 23, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Aug 23, 2017

jiayingz mentioned this pull request Aug 23, 2017

Implementation of the Device Plugin Manager #50482

Closed

RenaudWasTaken reviewed Aug 23, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2017

vikaschoudhary16 suggested changes Aug 24, 2017

View reviewed changes

vikaschoudhary16 reviewed Aug 24, 2017

View reviewed changes

jiayingz force-pushed the deviceplugin-jiayingz branch 10 times, most recently from a1708ad to 6edf47b Compare August 25, 2017 17:55

k8s-ci-robot assigned vishh Aug 25, 2017

vikaschoudhary16 suggested changes Aug 28, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

jiayingz force-pushed the deviceplugin-jiayingz branch from cbc13fe to bc942f5 Compare September 1, 2017 17:58

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 1, 2017

Added Device Plugin Manager

b563101

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

Renaud Gaubert and others added 4 commits September 1, 2017 11:47

Device Plugin Kubelet integration

c4a1c97

Testing

f7f4515

Alpha feature integration

7a8ad49

Kubelet side extension to support device allocation

02001af

jiayingz force-pushed the deviceplugin-jiayingz branch from bc942f5 to 02001af Compare September 1, 2017 18:56

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

vishh mentioned this pull request Sep 2, 2017

Deviceplugin checkpoint #51744

Merged

abgworrall modified the milestone: v1.8 Sep 2, 2017

k8s-github-robot merged commit 139e527 into kubernetes:master Sep 2, 2017

sjenning mentioned this pull request Sep 2, 2017

CPU manager static policy #51180

Merged

This was referenced Sep 3, 2017

cross-build: PR 51209 causing cross build failure #51863

Closed

pull-kubernetes-cross not being triggered on files it should be kubernetes/test-infra#4354

Closed

vikaschoudhary16 mentioned this pull request Sep 5, 2017

Device(smart-NICs) Plugin: Fix host networking limitation #51938

Closed

RenaudWasTaken mentioned this pull request Sep 8, 2017

Device plugin code cleanup #51993

Closed

resouer reviewed Sep 28, 2017

View reviewed changes

vikaschoudhary16 mentioned this pull request Apr 16, 2018

Add vikaschoudhary16 to the approvers in device manager #62184

Closed

vikaschoudhary16 mentioned this pull request Apr 24, 2018

Pod details missing in the Allocate DPAPI #63048

Closed

SergeyKanzhelev mentioned this pull request Sep 29, 2022

Sandeep issue todo resolved nedd of error defintions #112381

Closed


		}

		func (e endpoint) allocate(devs []string) (pluginapi.AllocateResponse, error) {

Deviceplugin jiayingz #51209

Deviceplugin jiayingz #51209

Conversation

jiayingz commented Aug 23, 2017

k8s-ci-robot commented Aug 23, 2017

Choose a reason for hiding this comment

RenaudWasTaken Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RenaudWasTaken Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RenaudWasTaken Aug 23, 2017 • 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

cmluciano commented Aug 23, 2017

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

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

Choose a reason for hiding this comment

RenaudWasTaken Aug 24, 2017 • 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

vikaschoudhary16 Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

jiayingz Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiayingz commented Aug 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikaschoudhary16 Aug 28, 2017 • 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

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

jiayingz commented Sep 1, 2017

vishh commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 2, 2017

Choose a reason for hiding this comment

RenaudWasTaken Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

RenaudWasTaken Aug 23, 2017 •

edited

Loading

RenaudWasTaken Aug 23, 2017 •

edited

Loading

RenaudWasTaken Aug 23, 2017 •

edited

Loading

RenaudWasTaken Aug 24, 2017 •

edited

Loading

vikaschoudhary16 Aug 24, 2017 •

edited

Loading

jiayingz Aug 24, 2017 •

edited

Loading

vikaschoudhary16 Aug 28, 2017 •

edited

Loading

RenaudWasTaken Sep 28, 2017 •

edited

Loading