Add EmptyDir volume and container overlay capacity isolation #45686

jingxu97 · 2017-05-11T20:57:13Z

This PR adds the support for isolating the emptyDir volume use. If user
sets a size limit for emptyDir volume, kubelet's eviction manager monitors its usage
and evict the pod if the usage exceeds the limit.

This feature is part of local storage capacity isolation and described in the proposal kubernetes/community#306

Release note:

Alpha feature: allows users to set storage limit to isolate EmptyDir volumes. It enforces the limit by evicting pods that exceed their storage limits

NickrenREN · 2017-05-17T03:21:58Z

pkg/kubelet/eviction/eviction_manager.go

+				if used != nil && size.Cmp(*zero) > 0 && used.Cmp(size) > 0 {
+					// the emptyDir usage exceeds the size limit, evict the pod
+					gracePeriod := int64(0)
+					m.evictPod(pod, v1.ResourceName("EmptyDir"), gracePeriod, fmt.Sprintf("emptyDir usage exceeds the limit %q", size.String()))


err = m.evictPod(...) ?

NickrenREN · 2017-05-17T03:22:30Z

test/e2e_node/emptyDir_eviction_test.go

@@ -0,0 +1,213 @@
+/*
+Copyright 2016 The Kubernetes Authors.


s/2016/2017

Files should not have capital letters in their names

thockin

I did not review all the eviction logic, just the API

thockin · 2017-05-17T16:44:51Z

pkg/api/v1/types.go

@@ -668,6 +668,11 @@ type EmptyDirVolumeSource struct {
 	// More info: http://kubernetes.io/docs/user-guide/volumes#emptydir
 	// +optional
 	Medium StorageMedium `json:"medium,omitempty" protobuf:"bytes,1,opt,name=medium,casttype=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.


Don't document the default as "nil" (that's a go-ism). Instead document as "If not specified, the default is to not enforce a limit". Though we might want to loosen this and say "If not specified, the default limit is implementation defined". Default tmpfs is (I think) 1/2 physical mem. Default disk is all storage, etc. I don't think we need to define that here, though.

thockin · 2017-05-17T16:45:33Z

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"))


"is disabled"

thockin · 2017-05-17T16:46:23Z

test/e2e_node/emptyDir_eviction_test.go

@@ -0,0 +1,213 @@
+/*
+Copyright 2016 The Kubernetes Authors.


Files should not have capital letters in their names

ddysher · 2017-05-23T06:37:56Z

pkg/kubelet/eviction/eviction_manager.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/client-go/util/clock"
 	"k8s.io/kubernetes/pkg/api/v1"
 	"k8s.io/kubernetes/pkg/features"
+	statsapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/stats"


Shouldn't this be k8s.io/kubernetes/pkg/kubelet/apis/stats/v1alpha1?

Thanks for catching it. fix it. strange it was working in this way too...

ddysher · 2017-05-23T06:42:44Z

pkg/kubelet/eviction/eviction_manager.go

+					glog.Errorf("eviction manager: pod %s exceeds emptyDir sizeLimit but failed to evict %v", format.Pod(pod), err)
+				} else {
+					glog.Infof("eviction manager: pod %s exceeds emptyDir sizeLimit and is evicted successfully", format.Pod(pod))
+					evict = true


Why can't we return early since the pod is already evicted?

The logic here is to evict all pods that violate the size limit for emptyDir volume. So we have to continue with the loop even after some pods are evicted.

Actually, you are right, return early is fine.

dashpole · 2017-05-30T14:02:51Z

pkg/kubelet/eviction/eviction_manager.go

+
+	// evict pods if there is a resource uage violation from local volume temporary storage
+	// If eviction happenes in localVolumeEviction function, skip the rest of eviction action
+	if m.localVolumeEviction(activePods) {


@vishh @derekwaynecarr, any preference as to whether doing local volume evictions is part of the same control loop as the rest of evictions? The main advantage of having them together is to not perform other evictions when pods are already being evicted for exceeding their local volume storage space

dashpole · 2017-05-30T14:08:49Z

pkg/kubelet/eviction/eviction_manager.go

+	return evict
+}
+
+func (m *managerImpl) evictPod(pod *v1.Pod, resourceName v1.ResourceName, gracePeriod int64, evictMsg string) error {


gracePeriod is always passed as 0, so you dont need it as an argument.

will this change in the future?

It could, but adding a parameter to a function is easy enough. Better to leave it out for now.

dashpole · 2017-05-30T14:10:43Z

Changes to the eviction manager need unit testing as well.

dashpole

Just a couple small things

dashpole · 2017-05-30T18:39:19Z

pkg/kubelet/eviction/eviction_manager.go

+			size := source.EmptyDir.SizeLimit
+			used := podVolumeUsed[pod.Spec.Volumes[i].Name]
+			zero := resource.NewQuantity(int64(0), resource.BinarySI)
+			glog.Infof("Jing pod %s limit %v, used %v", format.Pod(pod), size, used)


This is probably left over from testing. Remove

dashpole · 2017-05-30T19:46:35Z

pkg/kubelet/eviction/eviction_manager.go

+			used := podVolumeUsed[pod.Spec.Volumes[i].Name]
+			zero := resource.NewQuantity(int64(0), resource.BinarySI)
+			glog.Infof("Jing pod %s limit %v, used %v", format.Pod(pod), size, used)
+			if used != nil && size.Cmp(*zero) > 0 && used.Cmp(size) > 0 {


instead of Cmp(*zero), I think you can use Sign

dashpole · 2017-05-30T19:52:01Z

pkg/kubelet/eviction/eviction_manager.go

+		if overlayThreshold, ok := thresholdsMap[containerStat.Name]; ok {
+			if overlayThreshold.Cmp(*rootfs) < 0 {
+				gracePeriod := int64(0)
+				err := m.evictPod(pod, v1.ResourceName("containerOverlay"), gracePeriod, fmt.Sprintf("container's overlay usage exceeds the limit %q", overlayThreshold.String()))


Since we are passing in a relevant message to evictPod, why not do the logging inside of evictPod using that message. evictPod doesnt need to return an error in this case, and we can avoid having essentially the same log messages duplicated in 3 places.

fixed, put logging inside of evictPod, and return bool to indicate whether pod is evicted or not

Does it matter whether the pod is evicted or not? I believe we set the pod's status to failed, so kubelet housekeeping will terminate it even if the eviction manager failed.
Essentially, we should treat any pod that we attempt to evict as evicted.

jingxu97 · 2017-05-31T21:53:38Z

addressed the comments. @dashpole PTAL

falenn · 2017-06-01T17:48:39Z

This message was created automatically by mail delivery software. A message that you sent could not be delivered to one or more of its recipients. This is a temporary error. The following address(es) deferred: [email protected] Domain imwiz.com has exceeded the max emails per hour (173/150 (115%)) allowed. Message will be reattempted later

…

------- This is a copy of the message, including all the headers. ------ Received: from github-smtp2-ext2.iad.github.net ([192.30.252.193]:45681 helo=github-smtp2b-ext-cp1-prd.iad.github.net) by box969.bluehost.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.87) (envelope-from <[email protected]>) id 1dGU8E-000QTR-8a for [email protected]; Thu, 01 Jun 2017 11:43:34 -0600 Date: Thu, 01 Jun 2017 10:43:23 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1496339003; bh=B+ObljmZ5DCGW8z6BDSL8CP5MjEMQL1mHGl1TzAqIgs=; h=From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=x1esfSNpWD2lAcEg08vJBK9OwfCGtIktXyti8PHS2KLK+16Uss1Cicap6D3ifx3vw 8hKw6JYQv06ZfW5qKB7R5hLJSYZOgVcsrtCD0LV6QKCo6JaQZVEBv+zNvqyjqgWHug z8+ij/hSpEeClA7TUWS1kEojUf5+HjqiZFUiJGbE= From: Kubernetes Submit Queue <[email protected]> Reply-To: kubernetes/kubernetes <[email protected]> To: kubernetes/kubernetes <[email protected]> Cc: Subscribed <[email protected]> Message-ID: <kubernetes/kubernetes/pull/45686/[email protected]> In-Reply-To: <kubernetes/kubernetes/pull/[email protected]> References: <kubernetes/kubernetes/pull/[email protected]> Subject: Re: [kubernetes/kubernetes] Add EmptyDir volume capacity isolation (#45686) Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="--==_mimepart_5930523b6b593_633a3f90a0a15c3c3415"; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: list X-GitHub-Sender: k8s-merge-robot X-GitHub-Recipient: falenn X-GitHub-Reason: subscribed List-ID: kubernetes/kubernetes <kubernetes.kubernetes.github.com> List-Archive: https://github.com/kubernetes/kubernetes List-Post: <mailto:[email protected]> List-Unsubscribe: <mailto:unsub+000ab60a4fc71667972ec148ee09c82ad73e7bf321f496c892cf000000011548143b92a169ce0d98d100@reply.github.com>, <https://github.com/notifications/unsubscribe/AAq2CvmN_qeNvOh8dgRVdyPYzVjFBsU5ks5r_vg7gaJpZM4NYi21> X-Auto-Response-Suppress: All X-GitHub-Recipient-Address: [email protected] X-Spam-Status: No, score=-5.0 X-Spam-Score: -49 X-Spam-Bar: ----- X-Ham-Report: Spam detection software, running on the system "box969.bluehost.com", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see root\@localhost for details. Content preview: /lgtm cancel //PR changed after LGTM, removing LGTM. @dashpole @jingxu97 @smarterclayton @thockin @vishh -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #45686 (comment) [...] Content analysis details: (-5.0 points, 4.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -5.0 RCVD_IN_DNSWL_HI RBL: Sender listed at http://www.dnswl.org/, high trust [192.30.252.193 listed in list.dnswl.org] -2.8 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [192.30.252.193 listed in wl.mailspike.net] -0.0 SPF_PASS SPF: sender matches SPF record 1.3 HTML_IMAGE_ONLY_24 BODY: HTML: images with 2000-2400 bytes of words 0.0 HTML_MESSAGE BODY: HTML included in message -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid 1.7 AWL AWL: Adjusted score from AWL reputation of From: address X-Spam-Flag: NO

----==_mimepart_5930523b6b593_633a3f90a0a15c3c3415 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit /lgtm cancel //PR changed after LGTM, removing LGTM. @dashpole @jingxu97 @smarterclayton @thockin @vishh

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #45686 (comment) ----==_mimepart_5930523b6b593_633a3f90a0a15c3c3415 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit <p>/lgtm cancel //PR changed after LGTM, removing LGTM. <a href="https://github.com/dashpole" class="user-mention">@dashpole</a> <a href="https://github.com/jingxu97" class="user-mention">@jingxu97</a> <a href="https://github.com/smarterclayton" class="user-mention">@smarterclayton</a> <a href="https://github.com/thockin" class="user-mention">@thockin</a> <a href="https://github.com/vishh" class="user-mention">@vishh</a></p> <p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="#45686 (comment)">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAq2Cpu3p24GLtJqTDUpNwFiScXHWZibks5r_vg7gaJpZM4NYi21">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/AAq2CqQ3xploWN3q-yY6cQY_hlRtkUWzks5r_vg7gaJpZM4NYi21.gif" width="1" /></p> <div itemscope itemtype="http://schema.org/EmailMessage"> <div itemprop="action" itemscope itemtype="http://schema.org/ViewAction"> <link itemprop="url" href="#45686 (comment)"></link> <meta itemprop="name" content="View Pull Request"></meta> </div> <meta itemprop="description" content="View this Pull Request on GitHub"></meta> </div> <script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kubernetes/kubernetes","title":"kubernetes/kubernetes","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kubernetes/kubernetes"}},"updates":{"snippets":[{"icon":"PERSON","message":"@k8s-merge-robot in #45686: /lgtm cancel //PR changed after LGTM, removing LGTM. @dashpole @jingxu97 @smarterclayton @thockin @vishh"}],"action":{"name":"View Pull Request","url":"#45686 (comment)"}}}</script> ----==_mimepart_5930523b6b593_633a3f90a0a15c3c3415--

k8s-github-robot · 2017-06-01T17:49:31Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/kubelet/eviction/OWNERS~~ [thockin,vishh]
~~test/e2e_node/OWNERS~~ [thockin,vishh]

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-01T18:40:11Z

@k8s-bot unit test this

jingxu97 · 2017-06-01T19:39:56Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

This PR adds two features: 1. add support for isolating the emptyDir volume use. If user sets a size limit for emptyDir volume, kubelet's eviction manager monitors its usage and evict the pod if the usage exceeds the limit. 2. add support for isolating the local storage for container overlay. If the container's overly usage exceeds the limit defined in container spec, eviction manager will evict the pod.

jingxu97 · 2017-06-05T20:16:15Z

@k8s-bot gce etcd3 e2e test this

jingxu97 · 2017-06-05T20:16:35Z

@k8s-bot pull-kubernetes-verify test this

jingxu97 · 2017-06-05T20:16:54Z

@k8s-bot kops aws e2e test this

k8s-ci-robot · 2017-06-05T21:09:43Z

@jingxu97: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
Jenkins unit/integration	f4cedab5e4c7481d41311a6b26eb984e21fcd476	link	`@k8s-bot unit test this`
Jenkins kops AWS e2e	f4cedab5e4c7481d41311a6b26eb984e21fcd476	link	`@k8s-bot kops aws e2e test this`
Jenkins Kubemark GCE e2e	f4cedab5e4c7481d41311a6b26eb984e21fcd476	link	`@k8s-bot kubemark e2e test this`
Jenkins Bazel Build	f4cedab5e4c7481d41311a6b26eb984e21fcd476	link	`@k8s-bot bazel test this`
Jenkins GCE Node e2e	f4cedab5e4c7481d41311a6b26eb984e21fcd476	link	`@k8s-bot node e2e test this`
Jenkins GCE etcd3 e2e	f4cedab5e4c7481d41311a6b26eb984e21fcd476	link	`@k8s-bot gce etcd3 e2e test this`
Jenkins verification	f4cedab5e4c7481d41311a6b26eb984e21fcd476	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.

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.

jingxu97 · 2017-06-06T03:20:44Z

@k8s-bot kops aws e2e test this

jingxu97 · 2017-06-06T03:20:55Z

rebased, add /lgtm back

k8s-github-robot · 2017-06-06T06:08:58Z

Automatic merge from submit-queue

andyxning · 2017-12-11T05:52:34Z

@jingxu97

Why we choose to evict a pod when the usage if exceed the limit?
IMO, we use the monitor and check logic to enforce this logic? Kubelet does not depends on any disk quota hard limit that underlying fs supports, i.e., xfs quota.

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

k8s-github-robot assigned thockin and smarterclayton May 11, 2017

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 11, 2017

NickrenREN reviewed May 17, 2017

View reviewed changes

thockin reviewed May 17, 2017

View reviewed changes

jingxu97 force-pushed the May/emptyDir branch from c6c2446 to f4cedab Compare May 18, 2017 21:35

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2017

ddysher reviewed May 23, 2017

View reviewed changes

jingxu97 assigned dashpole May 26, 2017

jingxu97 requested a review from dashpole May 26, 2017 19:05

jingxu97 added this to the v1.7 milestone May 26, 2017

jingxu97 force-pushed the May/emptyDir branch 2 times, most recently from adb9650 to 431baee Compare May 26, 2017 23:37

dashpole reviewed May 30, 2017

View reviewed changes

jingxu97 force-pushed the May/emptyDir branch from 431baee to e1b4aae Compare May 30, 2017 21:35

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

jingxu97 force-pushed the May/emptyDir branch from e1b4aae to 00f1b53 Compare May 31, 2017 20:32

jingxu97 force-pushed the May/emptyDir branch from 26571c5 to 7fe5fdb Compare June 1, 2017 17:27

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

jingxu97 force-pushed the May/emptyDir branch from 7fe5fdb to a35b7a4 Compare June 1, 2017 18:49

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

vishh mentioned this pull request Jun 2, 2017

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

Open

19 tasks

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

jingxu97 force-pushed the May/emptyDir branch from a35b7a4 to 0b13aee Compare June 5, 2017 19:07

k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 5, 2017

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

k8s-github-robot merged commit cb68132 into kubernetes:master Jun 6, 2017

jingxu97 mentioned this pull request Jun 9, 2017

Add Eviction when EmptyDir exceeds user's requested limit #43330

Closed

ddysher mentioned this pull request Jun 13, 2017

Questions about disk-accounting and disk storage limitation #34439

Closed

jingxu97 changed the title ~~Add EmptyDir volume capacity isolation~~ Add EmptyDir volume and container overlay capacity isolation Jun 30, 2017

jingxu97 mentioned this pull request Jul 12, 2017

Local storage prototype #43333

Closed

xiangpengzhao mentioned this pull request Aug 9, 2017

kubelet-v1.7.0: storage capacity is 0 #50322

Closed

jingxu97 mentioned this pull request Jan 26, 2019

Add size limit to tmpfs #63641

Closed

Add EmptyDir volume and container overlay capacity isolation #45686

Add EmptyDir volume and container overlay capacity isolation #45686

Conversation

jingxu97 commented May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashpole commented May 30, 2017

dashpole 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

jingxu97 May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented May 31, 2017

falenn commented Jun 1, 2017 via email

k8s-github-robot commented Jun 1, 2017

jingxu97 commented Jun 1, 2017

jingxu97 commented Jun 1, 2017

jingxu97 commented Jun 5, 2017

jingxu97 commented Jun 5, 2017

jingxu97 commented Jun 5, 2017

k8s-ci-robot commented Jun 5, 2017 • edited Loading

jingxu97 commented Jun 6, 2017

jingxu97 commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

andyxning commented Dec 11, 2017

jingxu97 commented May 11, 2017 •

edited

Loading

jingxu97 May 31, 2017 •

edited

Loading

k8s-ci-robot commented Jun 5, 2017 •

edited

Loading