Support for resource quota on extended resources #57302

lichuqiang · 2017-12-18T06:13:38Z

Which issue(s) this PR fixes :
Fixes #46639 #57300 for resource quota support

Special notes for your reviewer:
One thing to be determined is if it necessary to Explicitly prohibit defining limits for extended resources in quota, like we did for hugepages, as the resource is not allowed to overcommit.

Release note:

Support for resource quota on extended resources

/cc @jiayingz @vishh @derekwaynecarr

lichuqiang · 2017-12-18T06:20:20Z

/area hw-accelerators

tengqm

Overall, this looks okay to me.

tengqm · 2017-12-18T06:42:02Z

pkg/quota/evaluator/core/pods.go

@@ -244,13 +245,13 @@ func podComputeUsageHelper(requests api.ResourceList, limits api.ResourceList) a
 		result[api.ResourceLimitsEphemeralStorage] = limit
 	}
 	for resource, request := range requests {
-		if quota.ContainsPrefix(requestedResourcePrefixes, resource) {
+		if quota.ContainsPrefix(requestedResourcePrefixes, resource) || helper.IsExtendedResourceName(resource) {


This revision is kind of hijacking the hugepages logic for extended resources, although the call to maskResourceWithPrefix below is unnecessary. So I'd suggest we add two separate loops for extended resources with comments.
The current logic is unnecessarily coupling extended resources to hugepages. This is not good for future maintenance.

SGTM, done.

lichuqiang · 2017-12-18T07:21:18Z

@tengqm Not quite sure about your meaning by "it is NOT OKAY to have devices reused across all regular containers by default", I think we never plan to support that anywhere. Also I fail to see relationship between resourceQuota support and the restriction.
In my opinion, we already have a basic support for device reuse after #56818 in, and left #56943 for further discussion, which should not block us on resourceQuota support :)

vikaschoudhary16 · 2017-12-18T08:16:54Z

@lichuqiang As a practice and for the sake of convenience of others, may i request you to please update the consolidated RMWG PR/issues excel sheet with this PR, if have not already.

vikaschoudhary16 · 2017-12-18T08:17:01Z

/sig node

vikaschoudhary16 · 2017-12-18T08:20:05Z

One thing to be determined is if it necessary to Explicitly prohibit defining limits for extended resources in quota, like we did for hugepages, as the resource is not allowed to overcommit.

In the existing code even today, limits cannot be unequal to requests for the ER. If the containe spec mentions so, will fail at validation.

tengqm · 2017-12-18T08:24:44Z

@vikaschoudhary16 Links have been added: https://docs.google.com/spreadsheets/d/1YBxIy23SY1BkVrGReFRr4e2OsmsLtfV3ETXPNviqDaU/edit#gid=0&range=E5

lichuqiang · 2017-12-18T08:31:24Z

The device reuse logic inroduced by #56818 will have the Pod requesting only 1 resource. It means a2, r1, r2 will all share the single resource allocated to a1.
However, when computing quota, the current logic is max(sum(r1, r2), a1, a2). The result will be 2.

Nope, maybe you should take another look at #56818, I think we didn't introduce the mechanism to reuse resources between regular containers :)

lichuqiang · 2017-12-18T08:39:17Z

In the existing code even today, limits cannot be unequal to requests for the ER. If the containe spec mentions so, will fail at validation.

Yep, thus, as @derekwaynecarr suggested: "we can only worry about quota on requests for the moment since the resource is not burstable.". So I wonder if we need to prohibit defining limits for extended resources in the validation func IsStandardQuotaResourceName and remove the logic for limits in quota evaluator, which has no real impact on the function though.

tengqm · 2017-12-18T08:43:03Z

@lichuqiang Okay, after checking the code another time, I realized that I was misunderstanding the code. The said situation is not happening. I'm deleting my comments in case it confuses other reviewers.

pineking · 2017-12-18T08:55:48Z

@lichuqiang how to use this for GPU quota, are there some docs? do we need to enable deviceplugin feature gate?

lichuqiang · 2017-12-18T09:11:30Z

@pineking As the resource name of GPU(alpha.kubernetes.io/nvidia-gpu) is in format of extended resource, you don't need extra operation for resource quota.
But to enable GPU, you could either manage it through device plugin or the old way. Both of the two require you to enable certain feature gate
By the way, seems feature gate for device plugin has been removed and the feature is enable by default in v1.9.

pineking · 2017-12-18T09:18:08Z

@lichuqiang thanks, got it, I use the “old way” to enable GPU.

vikaschoudhary16 · 2017-12-18T09:47:16Z

code logic wise LGTM.

tengqm · 2017-12-19T06:47:40Z

Oh, sorry. Just realized that we may need to hold this before claiming we support quota for extended resources. I think it is a bug introduced in #56818. Take the following Pod spec as an example:

spec:
  initContainers:
    - name: A1
      resources: 
        requests: {nvidia.com/gpu: 2}
    - name: A2
      resources: 
        requests: {nvidia.com/gpu: 2}

  containers:
    - name: C
      resources:
        requests: {nvidia.com/gpu: 2}

The current device reuse logic will first recognize the 4 GPU requests from init containers, then reuse 2 of them to the regular container C. So the total GPU requests from the pod is 4.

However, the quota check logic today ( https://github.com/kubernetes/kubernetes/blob/master/pkg/quota/evaluator/core/pods.go#L322-L332 ) computes the resource requests differently. It will do
max(max(init-containers), sum(regular-containers)). That means quota checking will see the Pod requesting 2 GPUs in the example above because A2 is run only after A1 has completed.

This problem is being fixed (as a side-effect) in #53698.

lichuqiang · 2017-12-19T06:52:51Z

The current device reuse logic will first recognize the 4 GPU requests from init containers, then reuse 2 of them to the regular container C. So the total GPU requests from the pod is 4.

@tengqm Oh, I think you should take another look at #56818, device reuse between init containers is supported, we would only recognize 2 GPU in your case.

vikaschoudhary16 · 2017-12-19T09:57:06Z

@tengqm Agree with @lichuqiang, code walk through suggests current device reuse logic will count 2 GPUs only. For second init container, first devices will be picked from already allocated devices.

tengqm · 2017-12-19T11:34:55Z

Ah, yes. Walked through the code again. I didn't quite get the tricks to build device id unions. Seems not a problem then.

vikaschoudhary16 · 2017-12-27T11:18:03Z

pkg/quota/evaluator/core/pods_test.go

+					InitContainers: []api.Container{{
+						Resources: api.ResourceRequirements{
+							Requests: api.ResourceList{api.ResourceName("example.com/dongle"): resource.MustParse("3")},
+						},


Should Limits also be mentioned?

+1 to add Limits for consistency, even though it may not affect this test.

wgliang · 2018-02-07T16:00:59Z

@lichuqiang Perhaps you merge two commits will be better.

vishh · 2018-02-15T21:41:41Z

pkg/apis/core/v1/validation/validation.go

@@ -75,6 +75,10 @@ func validateContainerResourceName(value string, fldPath *field.Path) field.Erro
 		if !helper.IsStandardContainerResourceName(value) {
 			return append(allErrs, field.Invalid(fldPath, value, "must be a standard resource for containers"))
 		}
+	} else if !v1helper.IsDefaultNamespaceResource(v1.ResourceName(value)) {
+		if !v1helper.IsExtendedResourceName(v1.ResourceName(value)) {
+			return append(allErrs, field.Invalid(fldPath, value, "doesn't follow extended resource name standard"))


s/name/naming/g

vishh · 2018-02-15T21:49:32Z

pkg/apis/core/helper/helpers.go

+// 1. the resource name is not in the default namespace;
+// 2. resource name does not have "requests." prefix,
+// to avoid confusion with the convention in quota
+// 3. it satisfies the rules in IsQualifiedName() after converted into quota resource name
 func IsExtendedResourceName(name core.ResourceName) bool {


Are there any unit tests for this method? If so can you add test cases for each of the scenario you mention in the comment, including maximum length validation?

a follow-on w/ tests would be good for this.

vishh · 2018-02-15T21:50:49Z

pkg/apis/core/types.go

@@ -4159,6 +4159,8 @@ const (
 	// HugePages request, in bytes. (500Gi = 500GiB = 500 * 1024 * 1024 * 1024)
 	// As burst is not supported for HugePages, we would only quota its request, and ignore the limit.
 	ResourceRequestsHugePagesPrefix = "requests.hugepages-"
+	// Default resource requests prefix
+	DefaultResourceRequestsPrefix = "requests."


@derekwaynecarr Do we still need explicit types for first class resources or can we apply the logic this PR employs for first class resources too?

i suspect we could consolidate logic now to say any compute resource (cpu, memory, etc.) could support requests.* or limits.* syntax is overcommittable.

vishh · 2018-02-15T21:54:24Z

/approve

derekwaynecarr

thanks for this useful feature.

derekwaynecarr · 2018-02-16T23:30:49Z

pkg/apis/core/helper/helpers.go

+// 1. the resource name is not in the default namespace;
+// 2. resource name does not have "requests." prefix,
+// to avoid confusion with the convention in quota
+// 3. it satisfies the rules in IsQualifiedName() after converted into quota resource name
 func IsExtendedResourceName(name core.ResourceName) bool {


a follow-on w/ tests would be good for this.

derekwaynecarr · 2018-02-16T23:32:13Z

pkg/apis/core/types.go

@@ -4159,6 +4159,8 @@ const (
 	// HugePages request, in bytes. (500Gi = 500GiB = 500 * 1024 * 1024 * 1024)
 	// As burst is not supported for HugePages, we would only quota its request, and ignore the limit.
 	ResourceRequestsHugePagesPrefix = "requests.hugepages-"
+	// Default resource requests prefix
+	DefaultResourceRequestsPrefix = "requests."


i suspect we could consolidate logic now to say any compute resource (cpu, memory, etc.) could support requests.* or limits.* syntax is overcommittable.

derekwaynecarr · 2018-02-16T23:35:10Z

/approve

jiayingz · 2018-02-17T00:26:50Z

/retest pull-kubernetes-verify

jiayingz · 2018-02-17T00:28:23Z

/assign @thockin @dchen1107 for approval

k8s-ci-robot · 2018-02-17T00:28:23Z

@jiayingz: GitHub didn't allow me to assign the following users: for, approval.

Note that only kubernetes members and repo collaborators can be assigned.

In response to this:

/assign @thockin @dchen1107 for approval

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.

jiayingz · 2018-02-20T17:16:10Z

/retest pull-kubernetes-verify

dchen1107 · 2018-02-20T17:24:39Z

/approve

thanks for the feature!

k8s-ci-robot · 2018-02-20T17:24:46Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/apis/core/OWNERS~~ [dchen1107]
~~pkg/controller/OWNERS~~ [dchen1107,derekwaynecarr]
~~pkg/quota/OWNERS~~ [dchen1107,derekwaynecarr,vishh]
~~staging/src/k8s.io/api/OWNERS~~ [dchen1107]
~~test/e2e/scheduling/OWNERS~~ [dchen1107,vishh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2018-02-20T19:28:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-02-20T20:13:09Z

/test all

Tests are more than 96 hours old. Re-running tests.

k8s-github-robot · 2018-02-20T21:26:32Z

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

k8s-github-robot · 2018-02-20T22:10:45Z

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

pineking · 2018-02-23T07:16:04Z

@lichuqiang are there some docs to tell how to use this feature for extended resources, e.g. "nvidia.com/gpu"

lichuqiang · 2018-02-23T07:39:07Z

@pineking Not yet, I'll post a PR to update the quota doc soon.
Basically you could make use of the ER quota in the way you do for CPU/memory.
Note that only items with "requests." prefix in quota is allowed for ER.

Pod example:

apiVersion: v1
kind: Pod
metadata:
  name: test-pod
  labels:
    name: test-pod-applied
spec:
  containers:
  - name: kubernetes-pause
    image: gcr.io/google-containers/pause:2.0
    resources:
      requests:
        cpu: 300m
        memory: 1300Mi
        nvidia.com/gpu: "4"
      limits:
        cpu: 300m
        memory: 1300Mi
        nvidia.com/gpu: "4"

quota example:

apiVersion: v1
kind: ResourceQuota
metadata:
  name: quota1
spec:
  hard:
    cpu: 300m
    memory: 3900Mi
    requests.nvidia.com/gpu: 4

pineking · 2018-02-23T14:32:02Z

@lichuqiang I have tested this feature. It works! Thanks.

rohitagarwal003 · 2018-04-02T02:08:12Z

@lichuqiang Can you update the docs with how to set this?

lichuqiang · 2018-04-02T03:31:49Z

docs update PR posted: kubernetes/website#7936

k8s-ci-robot requested review from derekwaynecarr, jiayingz and vishh December 18, 2017 06:13

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 18, 2017

k8s-github-robot assigned timothysc and vishh Dec 18, 2017

lichuqiang mentioned this pull request Dec 18, 2017

Initial support for resource quota and limit range on extended resources #57300

Closed

2 tasks

k8s-ci-robot added the area/hw-accelerators label Dec 18, 2017

tengqm reviewed Dec 18, 2017

View reviewed changes

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Dec 18, 2017

vikaschoudhary16 suggested changes Dec 27, 2017

View reviewed changes

vishh reviewed Feb 15, 2018

View reviewed changes

derekwaynecarr approved these changes Feb 16, 2018

View reviewed changes

k8s-ci-robot assigned dchen1107 and thockin Feb 17, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2018

k8s-github-robot merged commit 228c991 into kubernetes:master Feb 20, 2018

yanniszark mentioned this pull request Mar 26, 2018

Support GPU for ML/AI framework kubernetes-retired/kube-batch#145

Closed

lichuqiang mentioned this pull request Apr 2, 2018

Add ER quota description in quota doc kubernetes/website#7936

Merged

embano1 mentioned this pull request Apr 2, 2018

Still needed with Quota Support in ER? xychu/throttle#1

Closed

vikaschoudhary16 mentioned this pull request Apr 16, 2018

Add vikaschoudhary16 to the approvers in device manager #62184

Closed

Support for resource quota on extended resources #57302

Support for resource quota on extended resources #57302

Conversation

lichuqiang commented Dec 18, 2017 • edited Loading

lichuqiang commented Dec 18, 2017

tengqm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichuqiang commented Dec 18, 2017 • edited Loading

vikaschoudhary16 commented Dec 18, 2017

vikaschoudhary16 commented Dec 18, 2017

vikaschoudhary16 commented Dec 18, 2017

tengqm commented Dec 18, 2017

lichuqiang commented Dec 18, 2017 • edited Loading

lichuqiang commented Dec 18, 2017 • edited Loading

tengqm commented Dec 18, 2017 • edited Loading

pineking commented Dec 18, 2017 • edited Loading

lichuqiang commented Dec 18, 2017 • edited Loading

pineking commented Dec 18, 2017

vikaschoudhary16 commented Dec 18, 2017

tengqm commented Dec 19, 2017

lichuqiang commented Dec 19, 2017 • edited Loading

vikaschoudhary16 commented Dec 19, 2017

tengqm commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgliang commented Feb 7, 2018 • 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

vishh commented Feb 15, 2018

derekwaynecarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Feb 16, 2018

jiayingz commented Feb 17, 2018

jiayingz commented Feb 17, 2018

k8s-ci-robot commented Feb 17, 2018

jiayingz commented Feb 20, 2018

dchen1107 commented Feb 20, 2018

k8s-ci-robot commented Feb 20, 2018

fejta-bot commented Feb 20, 2018

k8s-github-robot commented Feb 20, 2018

k8s-github-robot commented Feb 20, 2018

k8s-github-robot commented Feb 20, 2018

pineking commented Feb 23, 2018

lichuqiang commented Feb 23, 2018 • edited Loading

pineking commented Feb 23, 2018

rohitagarwal003 commented Apr 2, 2018

lichuqiang commented Apr 2, 2018

lichuqiang commented Dec 18, 2017 •

edited

Loading

tengqm left a comment •

edited

Loading

lichuqiang commented Dec 18, 2017 •

edited

Loading

lichuqiang commented Dec 18, 2017 •

edited

Loading

lichuqiang commented Dec 18, 2017 •

edited

Loading

tengqm commented Dec 18, 2017 •

edited

Loading

pineking commented Dec 18, 2017 •

edited

Loading

lichuqiang commented Dec 18, 2017 •

edited

Loading

lichuqiang commented Dec 19, 2017 •

edited

Loading

wgliang commented Feb 7, 2018 •

edited

Loading

lichuqiang commented Feb 23, 2018 •

edited

Loading