Fix device manager to scan resources.Requests #57278

vikaschoudhary16 · 2017-12-16T20:22:03Z

What this PR does / why we need it:
This PR makes device manager to scan resources.Requests from the container spec. Currently
it scans resources.Limits. For extended resources, it is not mandatory for resources.Limits to be present in the container spec and if Limits are present, validation logic ensures that Limits will always be equal to Requests.

Fixes #57276

Special notes for your reviewer:

Release note:

None

/sig node

/cc @ConnorDoyle @vishh @jiayingz @RenaudWasTaken @tengqm @resouer @mindprince

k8s-ci-robot · 2017-12-16T20:22:09Z

@vikaschoudhary16: GitHub didn't allow me to request PR reviews from the following users: tengqm, RenaudWasTaken.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:
This PR makes device manager to scan resources.Requests from the container spec. Currently
it scans resources.Limits. For extended resources, it is not mandatory for resources.Limits to be present in the container spec and if Limits are present, validation logic ensures that Limits will always be equal to Requests.

Fixes #57276

Special notes for your reviewer:

Release note:
None
/sig node

/cc @ConnorDoyle @vishh @jiayingz @RenaudWasTaken @tengqm @resouer @mindprince

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.

RenaudWasTaken · 2017-12-16T20:27:40Z

@vikaschoudhary16 the issue @ConnorDoyle opened was about the fact that we don't scan Requests but we still have to scan Limits too :)

ConnorDoyle · 2017-12-16T23:08:12Z

@RenaudWasTaken regarding

we still have to scan Limits

Per the docs, requests default to limits if those are explicitly defined. For example, the scheduler only inspects requests but it is able to properly place pods that only specify limit.

So, it appears that requests are guaranteed to be a subset of limits. You can verify the behavior like this:

$ kubectl create -f - <<POD
apiVersion: v1
kind: Pod
metadata:
  name: er-test
spec:
  containers:
  - image: busybox
    name: er-test
    resources:
      requests:
        cpu: 50m
      limits:
        cpu: 100m
        example.com/foo: 1
POD

Then fetch the same pod resource from the API server with kubectl:

$ kubectl get pod er-test -o json | jq ".spec.containers[0].resources"

{
  "limits": {
    "cpu": "100m",
    "example.com/foo": "1"
  },
  "requests": {
    "cpu": "50m",
    "example.com/foo": "1"
  }
}

In summary, I think it is safe to only inspect requests here and ignore limits. We will see any ER limit reflected in requests, and we are further guaranteed that ER limit is either undefined or equal to request.

ConnorDoyle · 2017-12-16T23:10:18Z

Thanks for posting a PR for this so quickly!

ConnorDoyle · 2017-12-16T23:15:41Z

pkg/kubelet/cm/deviceplugin/manager.go

@@ -550,7 +550,7 @@ func (m *ManagerImpl) allocateContainerResources(pod *v1.Pod, container *v1.Cont
 	podUID := string(pod.UID)
 	contName := container.Name
 	allocatedDevicesUpdated := false
-	for k, v := range container.Resources.Limits {
+	for k, v := range container.Resources.Requests {


@vikaschoudhary16 it's might not be obvious why we can safely ignore Limits. Could we add a comment near here explaining the rationale?

RenaudWasTaken · 2017-12-16T23:49:48Z

@ConnorDoyle agreed but isn't the behavior undefined if someone sets requests and limits ? Should this error out ?

ConnorDoyle · 2017-12-17T00:22:30Z

You can set both request and limit for extended resources, but they fail validation if they are not equal.

ConnorDoyle · 2017-12-17T05:41:44Z

/assign

ConnorDoyle · 2017-12-17T05:52:49Z

/approve
Will add LGTM after the requested comments have been added.

vikaschoudhary16 · 2017-12-17T07:44:30Z

@ConnorDoyle Done. PTAL!

ConnorDoyle · 2017-12-17T14:38:43Z

pkg/kubelet/cm/deviceplugin/manager.go

@@ -550,7 +550,10 @@ func (m *ManagerImpl) allocateContainerResources(pod *v1.Pod, container *v1.Cont
 	podUID := string(pod.UID)
 	contName := container.Name
 	allocatedDevicesUpdated := false
-	for k, v := range container.Resources.Limits {
+	// NOTE: Skipping the Resources.Lists is safe here because:


s/Lists/Limits

resouer · 2017-12-18T01:00:31Z

pkg/kubelet/cm/deviceplugin/manager.go

-	for k, v := range container.Resources.Limits {
+	// NOTE: Skipping the Resources.Lists is safe here because:
+	// 1. If container Spec mentions Limits only, implicitly Requests, equal to Limits, get added to Spec.
+	// 2. If container Spec mentions Limits, which are greater than or less than Requests, will fail at validation.


Sorry, I can't recall we have validation to verify 2) , limits can be greater than requests by design in scheduler ... or I misunderstood the comment here?

resouer

I can confirm using requests here is safe and reasonable from scheduling aspect, so code lgtm, but comments may need update.

resouer · 2017-12-18T01:12:55Z

pkg/kubelet/cm/deviceplugin/manager.go

@@ -550,7 +550,10 @@ func (m *ManagerImpl) allocateContainerResources(pod *v1.Pod, container *v1.Cont
 	podUID := string(pod.UID)
 	contName := container.Name
 	allocatedDevicesUpdated := false
-	for k, v := range container.Resources.Limits {
+	// NOTE: Skipping the Resources.Lists is safe here because:
+	// 1. If container Spec mentions Limits only, implicitly Requests, equal to Limits, get added to Spec.


The commas around equal to Limits confused me a little bit.

ConnorDoyle · 2017-12-18T06:52:19Z

/lgtm

k8s-ci-robot · 2017-12-18T06:52:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorDoyle, vikaschoudhary16

Associated issue: #57276

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

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

~~pkg/kubelet/cm/deviceplugin/OWNERS~~ [ConnorDoyle]

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-12-18T06:56:31Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-12-18T07:43:58Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

jiayingz · 2017-12-18T17:56:39Z

@vikaschoudhary16 we may consider to cherry-pick this PR to 1.9.1.

vishh · 2017-12-18T19:17:19Z

pkg/kubelet/cm/deviceplugin/manager.go

-	for k, v := range container.Resources.Limits {
+	// NOTE: Skipping the Resources.Limits is safe here because:
+	// 1. If container Spec mentions Limits only, implicitly Requests, equal to Limits, will get added to the Spec.
+	// 2. If container Spec mentions Limits, which are greater than or less than Requests, will fail at validation.


What if limits are not set? I think this change should validate if requests==limits prior to using requests.

@vishh if requests != limits for ERs, it will fail here on validation step:
https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L4380
And if limits are not set, then it should not be a problem because now with this change, requests are being scanned.

Or i missed your question?

@vishh Yeah, I raised the same question before, while found that it is actually ensured by validation indeed.

That's why I suggested to update this comment to make it more clear. 😆

vishh · 2017-12-18T19:19:37Z

I'd appreciate if API behavioral changes are soaked for a bit before being merged.

ConnorDoyle · 2017-12-19T05:05:49Z

This will be reverted, per discussion here: #57170 (comment)

@vishh

Automatic merge from submit-queue (batch tested with PRs 57591, 57369). 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>. Revert back #57278 **What this PR does / why we need it**: This PR reverts back to behavior of scanning Limits. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Related # #57276 #57170 **Special notes for your reviewer**: **Release note**: ```release-note None ``` /sig node /cc @vishh @ConnorDoyle @jiayingz

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Dec 16, 2017

k8s-ci-robot requested a review from resouer December 16, 2017 20:22

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 16, 2017

k8s-ci-robot requested review from rohitagarwal003, ConnorDoyle, vishh and jiayingz December 16, 2017 20:22

k8s-github-robot assigned rohitagarwal003 Dec 16, 2017

ConnorDoyle reviewed Dec 16, 2017

View reviewed changes

k8s-ci-robot assigned ConnorDoyle Dec 17, 2017

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2017

vikaschoudhary16 force-pushed the limit branch from 13a02dd to fc0c937 Compare December 17, 2017 07:43

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

ConnorDoyle suggested changes Dec 17, 2017

View reviewed changes

resouer reviewed Dec 18, 2017

View reviewed changes

vikaschoudhary16 force-pushed the limit branch from fc0c937 to 02f7d97 Compare December 18, 2017 03:33

Look for requested resources in the Requests

bf1fb46

vikaschoudhary16 force-pushed the limit branch from 02f7d97 to bf1fb46 Compare December 18, 2017 03:57

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

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

k8s-github-robot merged commit 0a55f41 into kubernetes:master Dec 18, 2017

ConnorDoyle mentioned this pull request Dec 18, 2017

Device manager only considers extended resource limits when allocating devices. #57276

Closed

vishh reviewed Dec 18, 2017

View reviewed changes

vikaschoudhary16 deleted the limit branch December 18, 2017 21:07

vikaschoudhary16 added a commit to vikaschoudhary16/kubernetes that referenced this pull request Dec 22, 2017

Revert back kubernetes#57278

8749c5c

vikaschoudhary16 mentioned this pull request Apr 16, 2018

Add vikaschoudhary16 to the approvers in device manager #62184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix device manager to scan resources.Requests #57278

Fix device manager to scan resources.Requests #57278

vikaschoudhary16 commented Dec 16, 2017

k8s-ci-robot commented Dec 16, 2017

RenaudWasTaken commented Dec 16, 2017 •

edited

Loading

ConnorDoyle commented Dec 16, 2017 •

edited

Loading

ConnorDoyle commented Dec 16, 2017

ConnorDoyle Dec 16, 2017

vikaschoudhary16 Dec 17, 2017

RenaudWasTaken commented Dec 16, 2017

ConnorDoyle commented Dec 17, 2017

ConnorDoyle commented Dec 17, 2017

ConnorDoyle commented Dec 17, 2017

vikaschoudhary16 commented Dec 17, 2017

ConnorDoyle Dec 17, 2017

resouer Dec 18, 2017

resouer left a comment •

edited

Loading

resouer Dec 18, 2017 •

edited

Loading

ConnorDoyle commented Dec 18, 2017

k8s-ci-robot commented Dec 18, 2017

k8s-github-robot commented Dec 18, 2017

k8s-github-robot commented Dec 18, 2017

jiayingz commented Dec 18, 2017

vishh Dec 18, 2017

vikaschoudhary16 Dec 18, 2017

resouer Dec 19, 2017 •

edited

Loading

vishh commented Dec 18, 2017

ConnorDoyle commented Dec 19, 2017

Fix device manager to scan resources.Requests #57278

Fix device manager to scan resources.Requests #57278

Conversation

vikaschoudhary16 commented Dec 16, 2017

k8s-ci-robot commented Dec 16, 2017

RenaudWasTaken commented Dec 16, 2017 • edited Loading

ConnorDoyle commented Dec 16, 2017 • edited Loading

ConnorDoyle commented Dec 16, 2017

ConnorDoyle Dec 16, 2017

Choose a reason for hiding this comment

vikaschoudhary16 Dec 17, 2017

Choose a reason for hiding this comment

RenaudWasTaken commented Dec 16, 2017

ConnorDoyle commented Dec 17, 2017

ConnorDoyle commented Dec 17, 2017

ConnorDoyle commented Dec 17, 2017

vikaschoudhary16 commented Dec 17, 2017

ConnorDoyle Dec 17, 2017

Choose a reason for hiding this comment

resouer Dec 18, 2017

Choose a reason for hiding this comment

resouer left a comment • edited Loading

Choose a reason for hiding this comment

resouer Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

ConnorDoyle commented Dec 18, 2017

k8s-ci-robot commented Dec 18, 2017

k8s-github-robot commented Dec 18, 2017

k8s-github-robot commented Dec 18, 2017

jiayingz commented Dec 18, 2017

vishh Dec 18, 2017

Choose a reason for hiding this comment

vikaschoudhary16 Dec 18, 2017

Choose a reason for hiding this comment

resouer Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

vishh commented Dec 18, 2017

ConnorDoyle commented Dec 19, 2017

RenaudWasTaken commented Dec 16, 2017 •

edited

Loading

ConnorDoyle commented Dec 16, 2017 •

edited

Loading

resouer left a comment •

edited

Loading

resouer Dec 18, 2017 •

edited

Loading

resouer Dec 19, 2017 •

edited

Loading