Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Device plugin should only handle allocations of resources managed by it #53548

Closed
yguo0905 opened this issue Oct 6, 2017 · 6 comments
Closed

Device plugin should only handle allocations of resources managed by it #53548

yguo0905 opened this issue Oct 6, 2017 · 6 comments
Assignees
@jiayingz
Labels
kind/bug Categorizes issue or PR as related to a bug. milestone/removed sig/node Categorizes an issue or PR as relevant to SIG Node.

Comments

@yguo0905
Copy link
Contributor

yguo0905 commented Oct 6, 2017

The resources managed by device plugin are a subset of the extended resources. The device plugin should only handle the resources managed by it, instead of all extended resources

kubernetes/pkg/kubelet/cm/device_plugin_handler.go

Line 169 in 2d16260

if !deviceplugin.IsDeviceName(k) || needed == 0 {

kubernetes/pkg/kubelet/deviceplugin/utils.go

Lines 57 to 61 in 2d16260

// IsDeviceName returns whether the ResourceName points to an extended resource
// name exported by a device plugin.
func IsDeviceName(k v1.ResourceName) bool {
return v1helper.IsExtendedResourceName(k)
}

/kind bug
/assign @jiayingz
/cc @vishh
@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 6, 2017
@k8s-github-robot k8s-github-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Oct 6, 2017
@yguo0905
Copy link
Contributor Author

yguo0905 commented Oct 6, 2017

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Oct 6, 2017
@k8s-github-robot k8s-github-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Oct 6, 2017
@jiayingz jiayingz mentioned this issue Oct 6, 2017
Merged
@jiayingz
Copy link
Contributor

jiayingz commented Oct 6, 2017

Thanks a lot for reporting this issue, @yguo0905 ! I have a PR #53547 that hopefully can resolve this issue for 1.8. We still need to better handle device plugin resource cleanup in 1.9, which will be tracked in issue #53395.

@jiayingz
Copy link
Contributor

jiayingz commented Oct 6, 2017

/cc @vishh @mindprince @ConnorDoyle

@jiayingz
Copy link
Contributor

jiayingz commented Oct 6, 2017

@vishh could you add this bug to the 1.8 milestone? Thanks a lot!

@dchen1107 dchen1107 added this to the v1.9 milestone Oct 9, 2017
@vishh vishh mentioned this issue Oct 9, 2017
Merged
jiayingz added a commit to jiayingz/kubernetes that referenced this issue Oct 9, 2017
@jiayingz
Extends TestPodContainerDeviceAllocation() to expose issue kubernetes…
9b5e6a4 
…#53548

that container allocation request may fail if it requests some
extended resource not managed by any device plugin.
@jiayingz jiayingz mentioned this issue Oct 9, 2017
Closed
k8s-github-robot pushed a commit that referenced this issue Oct 9, 2017
Merge pull request #53547 from jiayingz/deviceplugin-fix
c12dab3 
Automatic merge from submit-queue (batch tested with PRs 52662, 53547, 53588, 53573, 53599). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

In DevicePluginHandlerImpl.Allocate(), skips untracked extended resou…

…rces.

Otherwise, we would fail a Pod allocation request that has an extended
resource not managed by any device plugin.



**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #
#53548

**Special notes for your reviewer**:

**Release note**:

```release-note
Ignore extended resources that are not registered with kubelet
```
jiayingz added a commit to jiayingz/kubernetes that referenced this issue Oct 9, 2017
@jiayingz
Extends TestPodContainerDeviceAllocation() to expose issue kubernetes…
773dcad 
…#53548

that container allocation request may fail if it requests some
extended resource not managed by any device plugin.
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Removed

@jiayingz @yguo0905

Important: This issue was missing labels required for the v1.9 milestone for more than 3 days:

priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.

Help

@k8s-github-robot k8s-github-robot removed this from the v1.9 milestone Oct 13, 2017
@jiayingz
Copy link
Contributor

jiayingz commented Dec 1, 2017

Fixed with PR #53547
/close

jiayingz added a commit to jiayingz/kubernetes that referenced this issue Jan 3, 2018
@jiayingz
Extends TestPodContainerDeviceAllocation() to expose issue kubernetes…
353d766 
…#53548

that container allocation request may fail if it requests some
extended resource not managed by any device plugin.
@lukaszgryglicki lukaszgryglicki mentioned this issue Mar 1, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jiayingz jiayingz

Labels
kind/bug Categorizes issue or PR as related to a bug. milestone/removed sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dchen1107 @k8s-github-robot @k8s-ci-robot @yguo0905 @jiayingz