Add local storage support in Quota #49610

NickrenREN · 2017-07-26T04:03:10Z

Add local storage(scratch, overlay) support in quota

Release note:

Add local ephemeral storage support to Quota

/cc @ddysher @jingxu97

k8s-ci-robot · 2017-07-26T04:03:11Z

@NickrenREN: GitHub didn't allow me to request PR reviews from the following users: ddysher.

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

In response to this:

Add local storage(scratch, overlay) support in quota

Release note:
Add local storage(scratch, overlay) support in Quota
/cc @ddysher @jingxu97

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.

NickrenREN · 2017-07-26T04:10:01Z

/assign @jingxu97

ddysher

The change assumes EmptyDir.SizeLimit is a request for both requests and limits in terms of resource allocation. This is not what people would do to explicitly request storage.kubernetes.io/scratch or storage.kubernets.io/overlay, do we want to force the two resources to have the same request/limit as well?

ddysher · 2017-07-26T05:13:24Z

pkg/quota/evaluator/core/pods.go

@@ -115,10 +121,32 @@ func (p *podEvaluator) Constraints(required []api.ResourceName, item runtime.Obj
 	requiredSet := quota.ToSet(required)
 	missingSet := sets.NewString()
 	for i := range pod.Spec.Containers {
-		enforcePodContainerConstraints(&pod.Spec.Containers[i], requiredSet, missingSet)
+		if len(pod.Spec.Containers[i].VolumeMounts) == 0 {


This looks a little verbose and duplicate for both containers and init container check; why can't you just have enforcePodContainerConstraints takes care of checking volume mounts since you already passed container[i] into it?

Yes, but we also need podVolume info, so add pod to the parameters
Done, thanks

ddysher · 2017-07-26T05:39:06Z

pkg/quota/evaluator/core/pods.go

 	}
 	for i := range pod.Spec.InitContainers {
-		enforcePodContainerConstraints(&pod.Spec.InitContainers[i], requiredSet, missingSet)
+		if len(pod.Spec.InitContainers[i].VolumeMounts) == 0 {


What is the effect of having volumes but no volume mounts in Pod?

This pr is confined to the container level

ddysher · 2017-07-26T05:41:39Z

pkg/quota/evaluator/core/pods.go

 	requests := container.Resources.Requests
 	limits := container.Resources.Limits
 	containerUsage := podUsageHelper(requests, limits)
 	containerSet := quota.ToSet(quota.ResourceNames(containerUsage))
 	if !containerSet.Equal(requiredSet) {
 		difference := requiredSet.Difference(containerSet)
-		missingSet.Insert(difference.List()...)
+		if podVolume == nil {


can we simplify the logic to?

if podVolume == nil || !checkStorageScratchLimitedByEmptyDir(podVolume, difference)

ddysher · 2017-07-26T05:42:16Z

pkg/quota/evaluator/core/pods.go

+// checkStorageScratchLimitedByEmptyDir checks whether the missing resource is storage scratch and if it is set
+// by emptydir
+func checkStorageScratchLimitedByEmptyDir(podVolume *api.Volume, missingSet sets.String) bool {
+	//missingSet must be subset of {api.ResourceStorageScratch,api.ResourceRequestsStorageScratch,api.ResourceLimitsStorageScratch}


nit: add space after //

ddysher · 2017-07-26T05:54:48Z

pkg/quota/evaluator/core/pods.go

 	}
 	// InitContainers are run sequentially before other containers start, so the highest
 	// init container resource is compared against the sum of app containers to determine
 	// the effective usage for both requests and limits.
 	for i := range pod.Spec.InitContainers {
 		requests = quota.Max(requests, pod.Spec.InitContainers[i].Resources.Requests)
 		limits = quota.Max(limits, pod.Spec.InitContainers[i].Resources.Limits)
+		for _, volumeMount := range pod.Spec.InitContainers[i].VolumeMounts {


Shouldn't you compute request and limit first, before computing quota.Max?

Thanks, yes, we need to

ddysher · 2017-07-26T05:55:43Z

pkg/quota/evaluator/core/pods.go

@@ -245,13 +324,51 @@ func PodUsageFunc(obj runtime.Object) (api.ResourceList, error) {
 	for i := range pod.Spec.Containers {
 		requests = quota.Add(requests, pod.Spec.Containers[i].Resources.Requests)
 		limits = quota.Add(limits, pod.Spec.Containers[i].Resources.Limits)
+		for _, volumeMount := range pod.Spec.Containers[i].VolumeMounts {


Use a helper function to remove duplicated code for container and init container,

NickrenREN · 2017-07-26T07:45:49Z

Hope that i have addressed all your comments. @ddysher Thanks
ready for another review

guangxuli · 2017-07-26T14:46:33Z

pkg/quota/evaluator/core/pods.go

+				if podVolume.Name == volumeMount.Name {
+					if checkStorageScratchLimitedByEmptyDir(&podVolume, difference) {
+						scratchLimitedByEmpty = true
+					}


Return directly would be better after scratchLimitedByEmpty = true.

Yeah, thanks, done

guangxuli · 2017-07-26T15:28:41Z

/retest

jingxu97 · 2017-07-27T20:49:23Z

pkg/api/types.go

@@ -3693,10 +3693,22 @@ const (
 	ResourceRequestsMemory ResourceName = "requests.memory"
 	// Storage request, in bytes
 	ResourceRequestsStorage ResourceName = "requests.storage"
+	// Local Storage request for overlay filesystem, in bytes. (500Gi = 500GiB = 500 * 1024 * 1024 * 1024)
+	// The resource name for ResourceStorageOverlay is alpha and it can change across releases.
+	ResourceRequestsStorageOverlay ResourceName = "requests.storage.kubernetes.io/overlay"


Modify the comments to match the name

jingxu97 · 2017-07-27T22:09:51Z

@NickrenREN Thank you for the PR. The code is ok, but we have to address the following issues before checking in the code.

Since we are adding the new alpha API, we have to use feature gate to validate whether those APIs are enabled or not.
For quota resource like CPU and memory, the resource and request/limit is one to one mapping. But local storage is different. The scratch space is node-level resource that could be used by pod level emptyDir volume or by container's overlay (depending whether there is one root partition or a separate imagefs partition). So it is kind of confusing when we set quota on Scratch space. Should it only include emptyDir volume or also container's overlay? Do admins want fine grain control over emptyDir and container overlay on Quota or they only care about total local storage usage and set one quota for it?

I will open up an issue for discussion #49818

jingxu97 · 2017-07-28T21:58:15Z

This issue for discussing this feature is opened up #49818

saad-ali · 2017-08-03T16:50:48Z

+@vishh

NickrenREN · 2017-08-08T08:13:17Z

feature gate added @jingxu97

timothysc · 2017-08-14T02:29:58Z

/cc @derekwaynecarr

derekwaynecarr · 2017-08-15T15:40:08Z

there was a meeting to potentially simplify local storage management to just have a single resource.

i would like to hold this PR until the design is actually closed.

NickrenREN · 2017-08-24T08:32:01Z

/retest

jingxu97 · 2017-08-25T17:10:32Z

pkg/quota/evaluator/core/pods.go

 	api.ResourceLimitsCPU,
 	api.ResourceLimitsMemory,
+	api.ResourceLimitsEphemeralStorage,


why there are two ResourceLimitsEphemeralStorage

Sorry, my bad

jingxu97 · 2017-08-25T22:40:14Z

/lgtm

jingxu97 · 2017-08-25T22:40:20Z

/approve

jingxu97 · 2017-08-25T22:40:42Z

cc @dashpole

NickrenREN · 2017-08-25T22:49:51Z

cool, thanks @jingxu97 ,
In order to make checks turn green, i add ephemeral storage resource to standard resource sets in this pr. This may conflict with eviction pr.
When eviction pr gets merged, i will rebase this and ping you again.

NickrenREN · 2017-08-26T06:17:27Z

rebased. kindly ping @jingxu97 @dashpole for review

derekwaynecarr · 2017-08-26T06:39:35Z

this looks good to me now.

/lgtm
/approve no-issue

NickrenREN · 2017-08-26T06:47:14Z

Cool, thanks @derekwaynecarr

ddysher · 2017-08-28T02:09:04Z

LGTM. I think I've requested changes before but do not know how to remove the request.

thockin · 2017-08-29T06:26:04Z

/approve no-issue

k8s-github-robot · 2017-08-29T06:26:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NickrenREN, derekwaynecarr, jingxu97, thockin

Associated issue requirement bypassed by: derekwaynecarr, thockin

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/api/OWNERS~~ [thockin]
~~pkg/quota/OWNERS~~ [derekwaynecarr,thockin]

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

NickrenREN · 2017-08-29T06:27:04Z

/retest

k8s-github-robot · 2017-08-29T08:17:32Z

Automatic merge from submit-queue (batch tested with PRs 50932, 49610, 51312, 51415, 50705)

k8s-ci-robot requested a review from jingxu97 July 26, 2017 04:03

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

k8s-github-robot assigned smarterclayton and justinsb Jul 26, 2017

k8s-ci-robot assigned jingxu97 Jul 26, 2017

ddysher suggested changes Jul 26, 2017

View reviewed changes

NickrenREN force-pushed the local-isolation branch from 2771ba5 to b163ad7 Compare July 26, 2017 07:40

guangxuli reviewed Jul 26, 2017

View reviewed changes

NickrenREN force-pushed the local-isolation branch from b163ad7 to 9ee401c Compare July 26, 2017 15:06

jingxu97 reviewed Jul 27, 2017

View reviewed changes

NickrenREN force-pushed the local-isolation branch from 9ee401c to a7dd0dc Compare July 28, 2017 05:29

NickrenREN force-pushed the local-isolation branch from a7dd0dc to b35882a Compare August 8, 2017 08:10

NickrenREN force-pushed the local-isolation branch from b35882a to 0ba804d Compare August 8, 2017 10:33

k8s-ci-robot requested a review from derekwaynecarr August 14, 2017 02:29

derekwaynecarr self-assigned this Aug 15, 2017

derekwaynecarr added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 15, 2017

NickrenREN force-pushed the local-isolation branch from 0ba804d to 4f655ab Compare August 16, 2017 06:21

NickrenREN force-pushed the local-isolation branch from 4f655ab to 9b32b80 Compare August 24, 2017 06:46

k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 24, 2017

jingxu97 reviewed Aug 25, 2017

View reviewed changes

NickrenREN force-pushed the local-isolation branch from 9b32b80 to 0caa4da Compare August 25, 2017 22:26

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

jingxu97 requested a review from dashpole August 25, 2017 22:40

NickrenREN added 2 commits August 26, 2017 14:13

Add local storage support in Quota

a3c180e

Add feature gate for local storage quota

bc393e3

NickrenREN force-pushed the local-isolation branch from 0caa4da to bc393e3 Compare August 26, 2017 06:16

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2017

derekwaynecarr removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 26, 2017

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

derekwaynecarr added this to the v1.8 milestone Aug 26, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2017

vishh mentioned this pull request Aug 29, 2017

[Storage] Add support for capacity isolation of shared partitions #43607

Open

19 tasks

k8s-github-robot merged commit fe365b8 into kubernetes:master Aug 29, 2017

NickrenREN deleted the local-isolation branch August 29, 2017 08:34

Add local storage support in Quota #49610

Add local storage support in Quota #49610

Conversation

NickrenREN commented Jul 26, 2017 • edited Loading

k8s-ci-robot commented Jul 26, 2017

NickrenREN commented Jul 26, 2017

ddysher left a 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

NickrenREN commented Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guangxuli commented Jul 26, 2017

Choose a reason for hiding this comment

jingxu97 commented Jul 27, 2017 • edited Loading

jingxu97 commented Jul 28, 2017

saad-ali commented Aug 3, 2017

NickrenREN commented Aug 8, 2017

timothysc commented Aug 14, 2017

derekwaynecarr commented Aug 15, 2017

NickrenREN commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Aug 25, 2017

jingxu97 commented Aug 25, 2017

jingxu97 commented Aug 25, 2017

NickrenREN commented Aug 25, 2017

NickrenREN commented Aug 26, 2017

derekwaynecarr commented Aug 26, 2017

NickrenREN commented Aug 26, 2017

ddysher commented Aug 28, 2017

thockin commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

NickrenREN commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

NickrenREN commented Jul 26, 2017 •

edited

Loading

NickrenREN commented Jul 26, 2017 •

edited

Loading

jingxu97 commented Jul 27, 2017 •

edited

Loading