Add Local Storage Capacity Isolation API by jingxu97 · Pull Request #44785 · kubernetes/kubernetes

jingxu97 · 2017-04-21T22:13:14Z

This PR adds the new APIs to support storage capacity isolation as
described in the proposal https://github.com/kubernetes/community/pull/306

Add SizeLimit for emptyDir volume
Add scratch and overlay storage type used by container level or
node level

Release note:

Alpha feature: Local volume Storage Capacity Isolation allows users to set storage limit to isolate EmptyDir volumes, container storage overlay, and also supports allocatable storage for shared root file system.

k8s-reviewable · 2017-04-21T22:13:20Z

This change is

nikhiljindal · 2017-04-23T01:48:34Z

Are you sure you wanted to refer #306?

Someone from @kubernetes/sig-storage-api-reviews should be able to review this better.
Whoever wants to take this, please feel free to remove me as reviewer.

vishh · 2017-05-04T18:29:55Z

pkg/api/types.go

 	// +optional
 	Medium StorageMedium
+	// Total amount of local storage required for this directory.
+	// The default is nil which means that the directory can use all available local storage on the node.


Note this is applicable to "memory" medium as well. Clarify that in the comments.

its not always all available local storage on the node. maybe just say its undefined if omitted.

vishh · 2017-05-09T21:44:01Z

pkg/api/types.go

 	// Volume size, in bytes (e,g. 5Gi = 5GiB = 5 * 1024 * 1024 * 1024)
 	ResourceStorage ResourceName = "storage"
+	// Local Storage for overlay filesystem, in bytes. (500Gi = 500GiB = 500 * 1024 * 1024 * 1024)
+	ResourceStorageOverlay ResourceName = "storage.kubernetes.io/overlay"


Clarify that the resource names are alpha and that they can change across releases. This is important because a new concept of ResourceClasses is being discussed where not having a domain name in the user resource name request will be useful.

Apart from the domain part, it might be necessary to clarify 'overlay' to make sure people won't confused about the "overlay filesystem driver" and general overlay mechanism used in container, which I think we mean the later here.

@vishh -- why not just name it alpha like gpu?

i guess it doesnt matter if its behind the feature gate really.

That will require renaming configs post alpha.

vishh · 2017-05-09T21:45:00Z

pkg/api/validation/validation.go

 		numVolumes++
-		// EmptyDirs have nothing to validate
+		if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
+			unsetSizeLimit := resource.Quantity{}


How about validating Pod Spec resources?

add validation for ResourceRequirements which will be called by validateContainer

derekwaynecarr · 2017-05-12T01:37:03Z

pkg/api/validation/validation.go

+		if !utilfeature.DefaultFeatureGate.Enabled(features.LocalStorageCapacityIsolation) {
+			unsetSizeLimit := resource.Quantity{}
+			if unsetSizeLimit.Cmp(source.EmptyDir.SizeLimit) != 0 {
+				allErrs = append(allErrs, field.Forbidden(fldPath.Child("emptyDir").Child("sizeLimit"), "LocalStorageCapacityIsolation are disabled by feature-gate"))


nit: is disabled

thockin · 2017-05-17T00:30:47Z

/approve

jingxu97 · 2017-05-31T16:51:56Z

Rebased. @thockin @vishh @derekwaynecarr PTAL

derekwaynecarr · 2017-05-31T17:37:21Z

pkg/api/types.go

I want to understand how this field will work with memory backed volumes.

For example, if a pod has a memory request for 500Mi of memory, and has a memory backed volume of 1Gi, the total memory request is still 500Mi, correct? Users are responsible for ensuring that memory backed volumes are sized as part of their pod requests similar to how /dev/shm works? In addition, a BestEffort pod can have a memory backed volume that has a size limit without changing the pod qos?

Yes, the total memory request is still 500Mi. When specifying the size limit for emptydir, it does not change pod qos

derekwaynecarr · 2017-05-31T17:39:42Z

Assuming the size limit for memory backed volumes does not impact QoS or how the pod cgroups are managed, this is LGTM. The scheduler still schedules based on resource requirements only.

derekwaynecarr · 2017-05-31T17:41:16Z

/lgtm

vishh · 2017-05-31T18:38:24Z

pkg/api/validation/validation.go

nit: Make the error user friendly - SizeLimit field disabled by feature-gate for EmptyDir volumes

vishh

Just one nit, otherwise LGTM

vishh · 2017-05-31T18:39:58Z

I removed the LGTM label to fix the pending nit and add a note on how EmptyDir SizeLimits apply to memory backed volumes as discussed offline.

jingxu97 · 2017-05-31T19:02:47Z

@vishh updated the comment, PTAL

This PR adds the new APIs to support storage capacity isolation as described in the proposal kubernetes/community#306 1. Add SizeLimit for emptyDir volume 2. Add scratch and overlay storage type used by container level or node level

jingxu97 · 2017-05-31T20:11:19Z

updated the comments, add /lgtm back

k8s-ci-robot · 2017-05-31T21:30:14Z

@jingxu97: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GCI GCE e2e	`00d7eaf`	link	`@k8s-bot gci gce e2e test this`
Jenkins Bazel Build	3a0d746ac33a47f8f7c191b69a22bb81321cf966	link	`@k8s-bot bazel test this`
Jenkins unit/integration	3a0d746ac33a47f8f7c191b69a22bb81321cf966	link	`@k8s-bot unit test this`
Jenkins verification	3a0d746ac33a47f8f7c191b69a22bb81321cf966	link	`@k8s-bot verify test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

vishh · 2017-05-31T22:33:49Z

/lgtm

jingxu97 · 2017-06-01T00:10:51Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

k8s-github-robot · 2017-06-01T00:54:11Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Details

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

~~api/OWNERS~~ [thockin]
~~docs/OWNERS~~ [thockin]
~~federation/OWNERS~~ [thockin]
~~pkg/OWNERS~~ [thockin]
~~staging/src/k8s.io/client-go/pkg/api/OWNERS~~ [thockin]

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

jingxu97 · 2017-06-01T16:10:27Z

bump priority to make sure API change for this feature gets in

k8s-github-robot · 2017-06-01T16:12:19Z

Automatic merge from submit-queue

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

k8s-github-robot assigned nikhiljindal and mikedanese Apr 21, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 21, 2017

jingxu97 requested review from thockin and vishh April 21, 2017 22:13

mikedanese assigned saad-ali and unassigned mikedanese and nikhiljindal Apr 27, 2017

jingxu97 requested a review from derekwaynecarr May 2, 2017 18:50

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2017

vishh reviewed May 9, 2017

View reviewed changes

derekwaynecarr reviewed May 12, 2017

View reviewed changes

jingxu97 mentioned this pull request May 16, 2017

add options for EmptyDirVolumeSource to enable set size for medium of… #45656

Closed

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

jingxu97 force-pushed the April/apistorage branch 2 times, most recently from fc90e24 to 3a0d746 Compare May 17, 2017 21:38

jingxu97 force-pushed the April/apistorage branch from 3a0d746 to a34af13 Compare May 25, 2017 21:10

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

jingxu97 force-pushed the April/apistorage branch from 2218864 to 9a0a8a7 Compare May 31, 2017 16:50

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

derekwaynecarr reviewed May 31, 2017

View reviewed changes

k8s-ci-robot assigned derekwaynecarr May 31, 2017

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

vishh reviewed May 31, 2017

View reviewed changes

vishh approved these changes May 31, 2017

View reviewed changes

vishh removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

jingxu97 force-pushed the April/apistorage branch from 9a0a8a7 to 12059d3 Compare May 31, 2017 19:00

jingxu97 added 2 commits May 31, 2017 12:26

Add storage isolation API

85f030c

This PR adds the new APIs to support storage capacity isolation as described in the proposal kubernetes/community#306 1. Add SizeLimit for emptyDir volume 2. Add scratch and overlay storage type used by container level or node level

generated files

695f7be

jingxu97 force-pushed the April/apistorage branch from 12059d3 to 695f7be Compare May 31, 2017 20:06

jingxu97 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

k8s-ci-robot assigned vishh May 31, 2017

jingxu97 added retest-not-required labels Jun 1, 2017

k8s-github-robot merged commit 14a1cdd into kubernetes:master Jun 1, 2017

vishh mentioned this pull request Jun 2, 2017

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

Open

19 tasks

Conversation

jingxu97 commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-reviewable commented Apr 21, 2017

Uh oh!

nikhiljindal commented Apr 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thockin commented May 17, 2017

Uh oh!

jingxu97 commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented May 31, 2017

Uh oh!

derekwaynecarr commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vishh left a comment

Choose a reason for hiding this comment

Uh oh!

vishh commented May 31, 2017

Uh oh!

jingxu97 commented May 31, 2017

Uh oh!

jingxu97 commented May 31, 2017

Uh oh!

k8s-ci-robot commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishh commented May 31, 2017

Uh oh!

jingxu97 commented Jun 1, 2017

Uh oh!

k8s-github-robot commented Jun 1, 2017

Uh oh!

jingxu97 commented Jun 1, 2017

Uh oh!

k8s-github-robot commented Jun 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jingxu97 commented Apr 21, 2017 •

edited

Loading

k8s-ci-robot commented May 31, 2017 •

edited

Loading